Skip to content

fix(ed25519): reject public keys outside the prime-order subgroup - #86

Merged
srpatcha merged 1 commit into
embeddedos-org:masterfrom
Kartikey1306:fix/ed25519-low-order-keys
Sep 3, 2026
Merged

fix(ed25519): reject public keys outside the prime-order subgroup#86
srpatcha merged 1 commit into
embeddedos-org:masterfrom
Kartikey1306:fix/ed25519-low-order-keys

Conversation

@Kartikey1306

@Kartikey1306 Kartikey1306 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Rebased onto master and reduced to the crypto. It was stacked on #84/#85 and carried 578 lines across six files; it is now two files, 138 lines, and reviews on its own.

The bypass

An all-zero public key with an all-zero signature verifies against any message — and it is not the only pair that does. Ed25519 has eight low-order points; used as the public key, the verification equation can hold regardless of the message, so the signature is not weak, it is absent.

master identity-key verify: rc=0 -> ACCEPTED (forgery works)

eos_ed25519_verify() is what stands between eos_image_verify_signature() and a booted image. The verifier rejected a non-canonical S and a key off the curve and stopped there — being on the curve says nothing about order.

Credit where it is due

The formulation here is @muhammadburhandevv-hub's, from #57, which reached this before I did. I have adopted it over my own and the commit carries Co-authored-by. It is better in two ways:

for (i = 0; i < 32; i++) order_l[i] = (uint8_t)ORDER_L[i];
scalarmult(multiple, q, order_l);
return point_is_identity(multiple) && !point_is_identity(A);

It derives the byte scalar from the existing ORDER_L rather than writing L out a second time — a mistyped duplicate constant rejects valid keys, silently and only in the field — and it states both required conditions in one expression. My earlier version hand-typed ORDER_L_BYTES[32] and used two separate guards; that is gone.

Both conditions are needed: the identity's order is 1, which divides L, so [L]identity = identity and the multiply alone admits it.

What this PR adds that #57 and #92 do not

The sweep: all 64 combinations of the eight low-order encodings as the public key and as R, with S = 0, across eight messages.

The breadth is not thoroughness for its own sake. Which pair forges depends on k = SHA-512(R || A || M) mod L, so for the order-4 and order-8 points it depends on the message:

forged
master, this test's eight messages 16 of 64
with the fix 0 of 64

Measured, not asserted. The count moves with the message set — a different eight gives 29; one fixed message gives 6 — which is exactly why a test pinned to one pair and one message can pass against unfixed code.

That is not hypothetical, and it caught me: the suite already had test_ed25519_zero_pubkey_rejected and test_ed25519_zero_signature_rejected, and both passed while the bypass was open — each holds one input legitimate, and the forgery needs both. My own first identity test used R = 0, which is not one of the 16, so it passed against unfixed code and proved nothing. #57 picked the right cell (R = identity); I did not, and the sweep is what found that.

Verified both directions

A wrong subgroup test rejects valid keys silently, so RFC 8032 §7.1 was checked before and after:

TEST 1 / 2 / 3 valid signature   accepted
tampered message                 rejected
tampered signature bit           rejected
all-zero key + all-zero sig      rejected   (was accepted)
identity key                     rejected

12/12 in this suite, 20/20 repo-wide under -DEBLDR_SANITIZE=ON. Against the unfixed verifier the sweep fails on its first accepted pair:

[FAIL] test_ed25519.c:283: eos_ed25519_verify(sig, k_low_order[k], messages[m], ...) != EOS_OK

On the overlap with #57 and #92

#57 got here first and is approved; its only blocker is a one-file CMakeLists.txt conflict, which I resolved and posted there — including that a normal merge does not delete the 371 lines of tests everyone is worried about (verified: both files survive, one conflict, one file).

If #57 or #92 lands first, the implementation here becomes a no-op and this reduces to the sweep, which applies to either branch unchanged and is the part neither has.

🤖 Generated with Claude Code

@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@Kartikey1306

Copy link
Copy Markdown
Contributor Author

Independent validation of this change, built standalone against core/ed25519_verify.c + core/sha512.c on both sides.

Forgery probe — all eight points of order dividing 8, each tried over 256 messages with R = identity, S = 0. For a key of order n that signature verifies whenever n divides k = SHA-512(R‖A‖M), so roughly 1 message in n — a regression test pinned to one fixed message can pass against unfixed code, which is why this sweeps.

public key order master this PR
01000000… 1 (identity) 256 / 256 accepted 0 / 256
ECFFFFFF…7F 2 124 / 256 0 / 256
00000000…00 4 61 / 256 0 / 256
00000000…80 4 63 / 256 0 / 256
26E8958F… 8 23 / 256 0 / 256
C7176A70… 8 34 / 256 0 / 256
ECFFFFFF…FF non-canonical 126 / 256 0 / 256
EDFFFFFF…FF non-canonical 61 / 256 0 / 256

748 forged acceptances in 2048 attempts on master; 0 here. The hit rates land on 1/2, 1/4 and 1/8 as the group orders predict, which is a decent sign the probe is measuring the real thing rather than an artefact.

Counter-check — the guard does not over-reject. A check that refused every key would also show zero forgeries, so RFC 8032 §7.1 vectors, on this branch:

RFC8032 TEST 1 (empty message)   valid sig -> ACCEPTED (correct)
                                 tampered  -> rejected (correct)
RFC8032 TEST 2 (1-byte message)  valid sig -> ACCEPTED (correct)
                                 tampered  -> rejected (correct)
RFC8032 TEST 3 (2-byte message)  valid sig -> ACCEPTED (correct)
                                 tampered  -> rejected (correct)

Two notes for whoever reviews:

  1. The identity needs rejecting separately from the subgroup test. [L]identity = identity, so identity passes "multiply by L and require the identity" — order 1 divides L. The first row above is the one that catches a guard built only on the subgroup test.
  2. eos fixed this same defect in services/crypto/src/ed25519_verify.c (merged eos#57), and its comment records the same trap. That the two repos carry independent Ed25519 verifiers and only one was hardened is the reason this survived — worth a drift check on the crypto sources, separately from this PR.

Overlaps #57, which is CONFLICTING and has not run CI since 08-31.

@Kartikey1306

Copy link
Copy Markdown
Contributor Author

Pushed 666ccc3, which replaces the two tests I had with a measured sweep. The
reason is worth stating, because it is the trap @srpatcha described on #73 and I
walked into it.

My test_ed25519_identity_key_rejected asserted on an all-zero signature, so it
exercised the pair (A = identity, R = 0). That pair is not a forgery on the
unfixed code
— so the test passed against master and proved nothing. It
looked like a regression test and was not one.

Measured on the parent commit, sweeping all eight low-order encodings as the
public key against all eight as R, with S = 0, over eight messages:

     R=0  R=1  R=2  R=3  R=4  R=5  R=6  R=7
A=0   YES  YES    .    .  YES    .    .    .
A=1     .  YES    .    .    .    .    .    .
A=2   YES  YES    .    .  YES    .    .    .
A=3   YES  YES    .    .  YES    .    .    .
A=4     .  YES    .    .  YES    .    .    .
A=5   YES  YES    .    .  YES    .    .    .
A=6     .  YES    .    .    .    .    .    .
A=7     .    .    .    .    .    .    .    .

accepted 16 of 64 (key,R) pairs by at least one message

On this branch: 0 of 64. The RFC 8032 §7.1 vectors still verify, so a check
that simply rejected everything would not pass either.

Two things fall out of the grid that are not obvious from the issue text:

  • A=1, R=0 is blank. An identity test written the obvious way — identity key,
    zero signature — is vacuous. The pair that forges with an identity key is
    R = identity, which is what fix: restore host build and harden Ed25519 verification #57 uses; that test is correctly targeted and
    mine was not.
  • A=7 never forges. The non-canonical encoding above p is refused by
    unpackneg() before the subgroup check sees it, so it is covered today for a
    different reason. Worth knowing so nobody removes that path thinking the
    subgroup test now covers it.

Also added a second test putting a low-order key with a genuine signature
rather than S = 0, so the key alone is disqualifying and the check does not
quietly depend on the signature being degenerate too.

Verified: ctest 21/21, and 21/21 again under -DEBLDR_SANITIZE=ON
(ASan + UBSan). The sweep fails on the parent commit and passes here.

On overlap with #57 — that PR reaches the same conclusion on the implementation
and reached it first; the point_is_identity()-alongside-subgroup shape is the
same in both. The differences are that this branch is on a base that merges
cleanly (#57 currently conflicts on CMakeLists.txt) and that the test sweeps
the class rather than one member of it. If the maintainers prefer to take #57's
implementation, the sweep here applies to it unchanged and I am happy to send it
as a test-only PR against that branch instead.

Kartikey1306 added a commit to Kartikey1306/eBoot that referenced this pull request Sep 1, 2026
eos and eBoot each carry their own Ed25519 verifier. Different code,
opposite return conventions — eos returns 1 to accept, eBoot returns
EOS_OK — doing the same job on the same wire format. For a while only
eos rejected low-order public keys, and nothing in either repo could
notice: there is no shared build, and the implementations are far too
different for a source diff to say anything.

That divergence is why embeddedos-org#73 survived in eBoot after eos had already fixed
it. Fixing eBoot closes the hole; it does not stop the next crypto fix
in either repo from failing to reach the other.

What can be shared is the data. tests/vectors/ed25519_contract_vectors.h
is 76 vectors of pure test data with a SHA-256 over them:

  64  low-order keys — the eight points of order dividing 8, each over
      eight messages. A key of order n makes (R = identity, S = 0)
      verify whenever n divides SHA-512(R||A||M), about one message in
      n, so a test pinned to a single message passes against unfixed
      code.
   3  RFC 8032 section 7.1 signatures that must verify
   6  the same three with one bit flipped in R or in S
   3  the same three with S + L, non-canonical per RFC 8032 5.1.7

The intended twin is tests/test_ed25519_contract.c in eos, compiling the
byte-identical header. Each side runs its own verifier and prints the
digest; a change to one copy that does not reach the other shows up as
two different digests, which is the part a diff could never give.

The header is generated, so the corpus is reproducible rather than
transcribed:  python3 tools/gen_ed25519_contract_vectors.py

The three positive vectors are load-bearing and the test says so: a
verifier that refuses everything satisfies all 73 negative vectors, so
it also fails if the corpus ever loses its accept cases.

Verified:
  on this branch (with the embeddedos-org#86 fix) -> 76/76, 3 accepted, 73 refused
  against unfixed origin/master     -> 28 of 76 wrong, exit 1
  ctest                             -> 22/22 passed
  digest 1059febedae2b3e3dfaa1ef5b419fb37ed7f0d53b377a3250b53b95a546282a2
@Kartikey1306
Kartikey1306 force-pushed the fix/ed25519-low-order-keys branch from 666ccc3 to bee391e Compare September 2, 2026 05:26
An all-zero public key with an all-zero signature verifies against any
message, and it is not the only pair that does. Ed25519 has eight
low-order points; used as the public key, the verification equation can
hold regardless of the message, so the signature is not weak but absent.
eos_ed25519_verify() is what stands between eos_image_verify_signature()
and a booted image.

The verifier rejected a non-canonical S and a key off the curve, and
stopped there. Being on the curve says nothing about order.

Adds the subgroup test: [L]A must be the identity, and A must not itself
be the identity -- the identity's order is 1, which divides L, so the
multiply alone admits it. A arrives negated from unpackneg(); [L](-A) =
-[L]A and the identity is its own negation, so neither condition is
affected by the sign.

The formulation is taken from embeddedos-org#57 by @muhammadburhandevv-hub, which
reached this first: it derives the byte scalar from the existing ORDER_L
instead of writing L out a second time, and states both conditions in one
expression. A mistyped duplicate constant would reject valid keys and only
in the field, so having no second copy is worth more than it looks.

Tests sweep all 64 combinations of the eight low-order encodings as the
public key and as R, with S = 0, across eight messages. The breadth is
not thoroughness for its own sake: which pair forges depends on
k = SHA-512(R || A || M) mod L, so for the order-4 and order-8 points it
depends on the message. Against master, 16 of those 64 pairs are accepted
for at least one of these messages; with the fix, 0.

That is also why the suite's existing test_ed25519_zero_pubkey_rejected
and test_ed25519_zero_signature_rejected both passed while the bypass was
open -- each holds one input legitimate, and the forgery needs both. My
own first attempt at an identity case used R = 0, which is not one of the
16, so it passed against unfixed code and proved nothing.

RFC 8032 section 7.1 vectors 1-3 still verify and tampering is still
rejected, checked before and after: a wrong subgroup test rejects valid
keys silently.

12/12 in this suite, 20/20 repo-wide with EBLDR_SANITIZE=ON. Against the
unfixed verifier the sweep fails on its first accepted pair.

Co-authored-by: muhammadburhandevv-hub <muhammadburhandevv-hub@users.noreply.github.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Kartikey1306
Kartikey1306 force-pushed the fix/ed25519-low-order-keys branch from bee391e to 0f34513 Compare September 2, 2026 05:37
@Kartikey1306

Copy link
Copy Markdown
Contributor Author

Two additions, both measured rather than argued.

What the fix costs — this is a bootloader

Nobody in #57, #92 or here had stated the price of an extra scalarmult per verification. On this host, -O2, RFC 8032 test 2, 200 iterations:

per verification
master 1.540 ms
with the fix 2.378 ms

+54%. Worth saying plainly rather than leaving a maintainer to find it on a Cortex-M4 where the absolute numbers are an order of magnitude larger. It is still the right trade — a signature bypass costs more than 0.8 ms — but it is a real boot-time change and should be a decision, not a surprise.

Stack, which matters more than time on an MCU, is the happier result. -fstack-usage on eos_ed25519_verify:

master   2928 bytes
fix      2912 bytes

No increase — 16 bytes lower, because the compiler reuses the frame. point_is_identity costs 64 bytes as its own frame. So there is no stack headroom to find on the small-RAM boards.

The added code is freestanding-safe — no malloc, printf, stdlib, time or rand — so the arm-none-eabi cross build is unaffected. CI's stm32f4 leg confirms.

A thirteenth case, pinning what the check deliberately does not cover

The subgroup check guards the public key, not R. I checked whether that is an omission:

valid key (RFC test 2) + all 8 low-order encodings as R, S=0, 8 messages
master:   0 of 8 accepted
with fix: 0 of 8 accepted

Not forgeable either way, and the reason is structural: k = SHA-512(R || A || M) depends on R, so an attacker choosing R would have to solve for it. A low-order A collapses the equation for any message; a low-order R does not.

test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery pins that, so a later "R should be checked too" change reads as scope creep, and removing the key check because "R is the untrusted half" fails loudly.

The message list is now shared at file scope between both sweeps, with a comment that the 16-of-64 count belongs to exactly that list.

13/13 in this suite, 20/20 repo-wide. Against the unfixed verifier the sweep still fails on its first accepted pair. Two files, 175 lines.

@srpatcha
srpatcha merged commit a172a6d into embeddedos-org:master Sep 3, 2026
26 checks passed

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — eBoot#86 "fix(ed25519): reject public keys outside the prime-order subgroup"

head: 0f34513 author: Kartikey1306 ci: pass

Verdict: The guard is correct and the reasoning behind it — both conditions required, ORDER_L reused rather than transcribed, sign-insensitivity of the test — holds up. The bypass is still live on master, so this PR is needed. The problem is not the code: there are now at least three unmerged implementations of the same fix competing, and a Critical secure-boot bypass has stayed open while they queue.

Not repeating what is already on the thread: the #57 overlap, the identity-vs-subgroup trap, the over-rejection counter-check, the perf and stack measurements, the freestanding-safety argument, the low-order-R scope note, and the eos/eBoot verifier drift are all yours and all stand. This review only adds what is not there.

Findings

# Severity File:line Finding Recommended fix
1 High not this PR's code — repository state A third implementation of this fix exists, and the bypass is still unfixed on master. master is 13a7a02; git show master:core/ed25519_verify.c contains no subgroup check, no point_is_identity, and git show master:tests/unit/test_ed25519.c has no low-order or identity-key test at all. So the Critical secure-boot bypass described in #73 is live on the default branch as of this run. Meanwhile the org repository carries an unmerged branch origin/fix/ed25519-low-order-keys at 8b88125, "fix(crypto): reject Ed25519 public keys outside the prime-order subgroup", authored by srpatcha 2026-09-01 21:03:44 -0700, Refs #73, #57, co-authored with muhammadburhandevv-hub — a single commit on top of 13a7a02 adding public_key_is_valid_subgroup() plus two tests. That function is logically identical to this PR's key_has_prime_order(): same point_is_identity(multiple) && !point_is_identity(A), same order_l[] derived from ORDER_L, same call site immediately after unpackneg(). Only the name and comments differ. Counting #57, this PR, and 8b88125, three equivalent guards are queued and none has landed. This is a coordination failure, not a code defect, and it is the most urgent thing in this review: the exposure window is being held open by the review process rather than by any technical difficulty. A maintainer picks one implementation and merges it now, this week, ahead of the test-quality discussion. On the merits the three differ only in naming and in test strength, and this PR's test corpus is the strongest of the three (see the note under Proposed changes), so the cheapest path is to merge 8b88125 — it is already on the org remote and based directly on master — and then take this PR's tests/unit/test_ed25519.c on top as a test-only follow-up, which is the offer you already made for #57. Whichever is chosen, close the other two explicitly so a fourth does not appear.
2 Medium tests/unit/test_ed25519.c, k_low_order[8][32] and its header comment "The eight low-order point encodings" The array does not contain the eight low-order encodings. Present: 00…00 (order 4), 01 00…00 (identity), the two order-8 points ending …6d53fc05 and …92ac037a, ECFF…FF7F (order 2), plus p and p+1 as non-canonical reductions to y=0 and the identity. Absent: 00…0080y = 0 with the sign bit set, the fourth canonical low-order point, which your own first comment's table listed and the code then dropped — and the two sign-flipped order-8 encodings (…92ac03fa, …6d53fc85). And slot 7, D9FF…FF, is not a low-order point at all; it is an out-of-range encoding that unpackneg() refuses on canonicality, as you noted yourself ("A=7 never forges"). So the sweep covers seven low-order encodings, spends one of its eight slots on a non-member, and its comment asserts a completeness it does not have. Not a vulnerability — [L](-A) = -[L]A, so the guard rejects every sign variant regardless — but this is the regression guard for a Critical secure-boot bypass, and partial coverage of a claimed class is precisely the failure mode your own 666ccc3 comment is about. Add the three missing encodings, keep D9FF…FF but move it out of k_low_order into a separately named k_non_canonical[] with a comment saying it is refused earlier and by a different mechanism, and change the header comment to state exactly which encodings the array holds and why. The sweep becomes 11×11 or 8×8 plus a canonicality case; either is fine, the labelling is the point.
3 Low tests/unit/test_ed25519.c, test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery Inserted immediately below the /* ---- SHA-512, the hash Ed25519 is defined over (FIPS 180-4) ---- */ banner, so an Ed25519 test is filed under the SHA-512 section while the other two new tests sit correctly above it. In a file that is the audit record for a TCB primitive, the section headings are load-bearing. Move it above the banner, next to the other two low-order tests.
4 Low PR comment 2026-09-02T05:37:54Z, the latency table Master design §28.1 requires a latency claim to carry "Hardware, clock, configuration, compiler flags, measurement method and distribution." The +54% figure gives compiler flags (-O2), the workload (RFC 8032 test 2) and the iteration count (200), but names no hardware or clock and reports a single mean per side with no distribution. A 1.540 ms → 2.378 ms mean over 200 iterations on an unnamed host is not a §28.1-conforming benchmark, and this one is load-bearing — it is the number a maintainer will use to decide whether the boot-time cost is acceptable on a Cortex-M4. State the host CPU and clock, and give min/median/p95 rather than a mean. If a Cortex-M4 number is available from the stm32f4 CI leg or a board, that is the number that actually matters for the decision.
5 Low tests/unit/test_ed25519.c, comment in test_ed25519_low_order_forgeries_rejected "Measured against this file's parent commit, 16 of these 64 pairs are accepted" — the comment names no SHA, and "this file's parent commit" stops identifying anything once the branch is rebased or the guard lands. A future reader cannot reproduce or falsify the number. Name the commit: "measured against 13a7a02, the last commit before the subgroup check".

Architecture conformance

Conforms. §21 Tier 1 — Foundation; core/ per .ai/architect.md ("core/ shared boot logic"). No new include, link line or target_link_libraries entry, and no dependency added in any direction — the guard is built from scalarmult, point_pack, fe_copy16 and ORDER_L, all already in the same translation unit. §14.1 "do not invent cryptographic primitives" is satisfied: this is a standard subgroup membership test, not a new primitive, and it is derived from the existing group order rather than a fresh constant. §8 Stage 1 image verification is the correct home for it. §5.1 minimal TCB — 53 lines and one scalarmult, no growth in dependencies.

The structural problem this PR surfaces is not in the PR: eBoot and eos each carry an independent Ed25519 verifier, the same bypass existed in both, and only one was fixed for weeks. §14.1 says to use reviewed libraries and to integrate key management across eBoot, eSec, eOTA and release signing, but says nothing about a primitive having a single implementation — and §21 puts eBoot in Tier 1 and eSec in Tier 2, so eBoot depending on eSec's crypto would point up a tier and is forbidden by §5.1. The design therefore rules out the obvious deduplication while being silent on the alternative. You raised the drift itself, so I am not restating it; the design gap is #89's subject and I am recording the proposal against that PR rather than duplicating it here.

Finding 1 has a second design dimension worth naming, because it is not covered by that proposal. Nothing in the design says who adjudicates when three contributors independently fix the same Critical defect, or that an open bypass on the default branch takes priority over review polish on the fix for it. §36's Go/No-Go gates and §28's evidence policy both govern what may be claimed; neither governs latency between a Critical finding and a merge. That is the mechanism that kept this one open, and it is out of scope for a PR review to fix.

Proposed changes

  1. Merge one of the three implementations, now (finding 1), ahead of everything below. 8b88125 is the cheapest to land — one commit, already on the org remote, based directly on 13a7a02 — and this PR's tests are the strongest, so 8b88125 followed by a test-only version of this PR gets both. That ordering is a suggestion; the priority is not.
  2. Fix k_low_order and its comment (finding 2) — the three missing encodings and the misfiled non-canonical entry.
  3. Move the low-order-R test above the SHA-512 banner (finding 3).
  4. Name 13a7a02 in the sweep comment (finding 5).
  5. Re-state the latency figure to §28.1 (finding 4), in the PR body rather than the code.

One thing worth putting to the maintainers, because it decides which of the three to take. 8b88125's regression test, test_ed25519_low_order_keys_rejected, sweeps 4 encodings × 4 R values against one message, "untrusted firmware", and has no order-2 encoding (ECFF…FF7F) anywhere in the file. By your own measured table, the order-4 and order-8 rows forge for roughly one message in four and one in eight, so a single-message sweep is expected to miss most of its own pairs against unfixed code — the same vacuity you found in your first attempt and fixed in 666ccc3. So of the three queued fixes, the guards are equivalent and this PR's tests are materially better. Take the guard from whichever branch merges most easily and the corpus from here.

Not checked

  • Nothing was executed. No forgery sweep, no RFC 8032 vector, no ctest, no timing, no -fstack-usage. Reproducing any of it requires checking out the PR head in /home/srpatcha/eos/eBoot, which the run brief forbids. Every number in the PR body and thread — 748/2048 on master, 0/2048 here, the 16-of-64 grid, 13/13, 20/20, 1.540 ms/2.378 ms, 2928/2912 bytes — is the author's, unverified by me. The tables are internally consistent and the 1/2, 1/4, 1/8 hit rates do match the group orders, which is corroboration, not verification.
  • Finding 2 is from reading the byte arrays, not from computing point orders. I identified the missing encodings by comparing the code's array against the standard set of eight low-order Ed25519 encodings and against your own comment table, which listed 00…0080 where the code does not. I did not independently verify the order of any point by computation, and I did not confirm that D9FF…FF fails canonicality rather than being a low-order point — that rests on your "A=7 never forges" note.
  • Whether 8b88125's single-message test is actually vacuous for some pairs was not measured. It follows from your table if the table is right; I did not run it against a pre-fix build to see which of its 16 pairs pass for the wrong reason.
  • Repo-total test counts across this stack are inconsistent and unresolved. #84 says 20, #85 says 21, this PR's earlier comment says 21 and its latest says 20. I cannot tell from here which base each was run on.
  • 21 of 22 checks pass; Create GitHub Release reports skipping, expected on a PR. No required check is failing. No CI job is named for -DEBLDR_SANITIZE=ON, so the sanitizer claims rest on local runs.
  • This PR's actual head is not in the local clone and was never read as source. 0f345139 is not a valid object in /home/srpatcha/eos/eBoot even after this run's fetch — it lives in the contributor's fork. Everything I say about this PR's code comes from the bundle's diff.patch, not from a working tree. Separately, the local checkout is sitting on the org-side branch fix/ed25519-low-order-keys at 8b88125, which shares a name with this PR's head branch but is a different commit by a different author; the sync step correctly left it alone. An earlier draft of this review misread that checkout as master and reported the fix as already merged. It is not: master is 13a7a02 and carries no subgroup check. Findings 2-5 were read from the bundle diff and are unaffected. Corrected before publication; nothing was posted to GitHub, as this run is DRY_RUN=1.
  • Which PR 8b88125 belongs to was not established. It may be #92, which your 2026-09-02 comment references, or an unopened branch. Determining that needs the GitHub API, which I did not query.
  • Merge mechanics not attempted. mergeStateStatus: BLOCKED. I did not try rebasing this PR onto 8b88125 or onto 13a7a02, and cannot say whether the test-file hunks apply cleanly over 8b88125's added tests — they touch adjacent regions of tests/unit/test_ed25519.c and main(), so expect conflicts in both.

Automated architecture review of 0f345139b7a5 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

Kartikey1306 added a commit to Kartikey1306/eBoot that referenced this pull request Sep 3, 2026
eos and eBoot each carry their own Ed25519 verifier. Different code,
opposite return conventions — eos returns 1 to accept, eBoot returns
EOS_OK — doing the same job on the same wire format. For a while only
eos rejected low-order public keys, and nothing in either repo could
notice: there is no shared build, and the implementations are far too
different for a source diff to say anything.

That divergence is why embeddedos-org#73 survived in eBoot after eos had already fixed
it. Fixing eBoot closes the hole; it does not stop the next crypto fix
in either repo from failing to reach the other.

What can be shared is the data. tests/vectors/ed25519_contract_vectors.h
is 76 vectors of pure test data with a SHA-256 over them:

  64  low-order keys — the eight points of order dividing 8, each over
      eight messages. A key of order n makes (R = identity, S = 0)
      verify whenever n divides SHA-512(R||A||M), about one message in
      n, so a test pinned to a single message passes against unfixed
      code.
   3  RFC 8032 section 7.1 signatures that must verify
   6  the same three with one bit flipped in R or in S
   3  the same three with S + L, non-canonical per RFC 8032 5.1.7

The intended twin is tests/test_ed25519_contract.c in eos, compiling the
byte-identical header. Each side runs its own verifier and prints the
digest; a change to one copy that does not reach the other shows up as
two different digests, which is the part a diff could never give.

The header is generated, so the corpus is reproducible rather than
transcribed:  python3 tools/gen_ed25519_contract_vectors.py

The three positive vectors are load-bearing and the test says so: a
verifier that refuses everything satisfies all 73 negative vectors, so
it also fails if the corpus ever loses its accept cases.

Verified:
  on this branch (with the embeddedos-org#86 fix) -> 76/76, 3 accepted, 73 refused
  against unfixed origin/master     -> 28 of 76 wrong, exit 1
  ctest                             -> 22/22 passed
  digest 1059febedae2b3e3dfaa1ef5b419fb37ed7f0d53b377a3250b53b95a546282a2
Kartikey1306 added a commit to Kartikey1306/eBoot that referenced this pull request Sep 3, 2026
Answers the review on embeddedos-org#89.

Finding 1 (High) -- the digest was printf'd and never compared to anything,
so the drift guard the PR is named for did not exist. Worse, it could not:
EOS_ED25519_CONTRACT_DIGEST came from the header this repo's own generator had
just written, a hash taken over its own output. Editing the generator on one
side produced a self-consistent corpus with a new digest, a green test, and a
divergence visible only to a human reading two CI logs in two repositories.
Three places asserted the guarantee -- the PR body, this file's docblock, and
the generator's emitted header comment -- and none implemented it. The unused
<string.h> include was the strcmp that never got written.

Now pinned as a literal in committed source, with the vector count and the
accept count beside it, because those are generated too: a corpus that shrank
from 76 to 40, or lost two of its three RFC 8032 positives, previously passed.
positives == 0 only fired when the last positive went.

Finding 2 (Medium) -- tests/vectors/ed25519_contract_vectors.h is now
committed rather than generated into the build tree. What the TCB's signature
verifier was tested against should be visible from a tag. The reason it was
generated -- two repos holding the same file and keeping it in step by hand --
is what the pinned digest now handles. That also drops
find_package(Python3 ... REQUIRED), which had made a Python interpreter a hard
configure-time dependency of a pure-C test suite.

Finding 3 (Medium) -- embeddedos-org#86 has merged, so this branch is rebased onto master
and reduced to its own three files. It previously carried all of embeddedos-org#84 and embeddedos-org#85
(device tree parsing, +475 lines, unrelated to Ed25519) plus embeddedos-org#86's verifier
change, none of which was declared in the body.

Verified:
  ctest                                     22/22 PASS
  test_ed25519_contract                     76 vectors, 3 accept, 73 refuse
  the guard now discriminates: adding one vector to the generator gives
      [FAIL] corpus digest changed
             expected 1059febe...282a2
             got      e08b0632...28905
    and exit 1. Before this commit the same edit printed a different digest
    and exited 0.
  cmake configure with no Python on PATH     succeeds (no find_package)

Refs embeddedos-org#89
srpatcha pushed a commit that referenced this pull request Sep 8, 2026
…rged broken (#94)

master (22d8f8b) does not compile. Two independent double-merges, both the
same shape: two PRs fixing adjacent things landed on stale bases, each was
green on its own branch, and the result was never rebuilt.

1. include/eos_image.h — #93 replaced reserved[30] with tlv_len (2) +
   tlv_hash[28], preserving every offset. #87 merged afterwards carrying
   asserts written against the older struct:

     error: no member named 'reserved' in 'eos_image_header_t'   (x2)

   #93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert
   was a duplicate; the width assert had no replacement and is restored as two
   asserts covering both halves of the same 30-byte span. No offset moves and
   the wire format is unchanged.

2. core/ed25519_verify.c — #86 and #57 both landed a subgroup guard, so the
   file carried two byte-identical point_is_identity() definitions:

     error: redefinition of 'point_is_identity'

   Only #57's public_key_is_valid_subgroup() is wired to the call site, so
   #86's key_has_prime_order() was dead. Kept the live function, folded #86's
   fuller rationale onto it, deleted the duplicate.

3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of
   test_ed25519_identity_key_forgery_rejected, main() calling it twice and two
   tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery
   referencing k_low_order[] and messages[] that the merge had dropped.

   While restoring the corpus, corrected it (review finding on #86): the array
   claimed to hold "the eight low-order point encodings" and held five. Every
   order here was computed rather than copied — decode y, recover x, add the
   point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8,
   8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8
   encodings. D9FF..FF was in the array and is not a low-order point at all —
   no x satisfies the curve equation for that y — so it moves to a separate
   k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1).

   tests_run was assigned a literal (11) in main() and never incremented,
   which is how the duplicate call and the two unregistered tests went
   unnoticed. The TEST macro now increments it, so the total cannot drift.

Verified:
  cmake -DEBLDR_BUILD_TESTS=ON on master   FAILS to build, 3 errors
  same with this commit                    builds clean
  ctest                                    21/21 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   21/21 PASS
  pytest tests/                            24 passed, 1 skipped
  test_ed25519                             14/14 PASS (was 11 claimed, 12 run)
  discrimination, with `public_key_is_valid_subgroup` disabled:
    test_ed25519_low_order_keys_rejected   FAILS, as it must
    test_ed25519_non_canonical_...         still PASSES — those are refused by
      unpackneg() on canonicality, a different mechanism, which is the reason
      they are held in a separate array rather than counted among the eight.
srpatcha pushed a commit that referenced this pull request Sep 8, 2026
* fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken

master (22d8f8b) does not compile. Two independent double-merges, both the
same shape: two PRs fixing adjacent things landed on stale bases, each was
green on its own branch, and the result was never rebuilt.

1. include/eos_image.h — #93 replaced reserved[30] with tlv_len (2) +
   tlv_hash[28], preserving every offset. #87 merged afterwards carrying
   asserts written against the older struct:

     error: no member named 'reserved' in 'eos_image_header_t'   (x2)

   #93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert
   was a duplicate; the width assert had no replacement and is restored as two
   asserts covering both halves of the same 30-byte span. No offset moves and
   the wire format is unchanged.

2. core/ed25519_verify.c — #86 and #57 both landed a subgroup guard, so the
   file carried two byte-identical point_is_identity() definitions:

     error: redefinition of 'point_is_identity'

   Only #57's public_key_is_valid_subgroup() is wired to the call site, so
   #86's key_has_prime_order() was dead. Kept the live function, folded #86's
   fuller rationale onto it, deleted the duplicate.

3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of
   test_ed25519_identity_key_forgery_rejected, main() calling it twice and two
   tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery
   referencing k_low_order[] and messages[] that the merge had dropped.

   While restoring the corpus, corrected it (review finding on #86): the array
   claimed to hold "the eight low-order point encodings" and held five. Every
   order here was computed rather than copied — decode y, recover x, add the
   point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8,
   8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8
   encodings. D9FF..FF was in the array and is not a low-order point at all —
   no x satisfies the curve equation for that y — so it moves to a separate
   k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1).

   tests_run was assigned a literal (11) in main() and never incremented,
   which is how the duplicate call and the two unregistered tests went
   unnoticed. The TEST macro now increments it, so the total cannot drift.

Verified:
  cmake -DEBLDR_BUILD_TESTS=ON on master   FAILS to build, 3 errors
  same with this commit                    builds clean
  ctest                                    21/21 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   21/21 PASS
  pytest tests/                            24 passed, 1 skipped
  test_ed25519                             14/14 PASS (was 11 claimed, 12 run)
  discrimination, with `public_key_is_valid_subgroup` disabled:
    test_ed25519_low_order_keys_rejected   FAILS, as it must
    test_ed25519_non_canonical_...         still PASSES — those are refused by
      unpackneg() on canonicality, a different mechanism, which is the reason
      they are held in a separate array rather than counted among the eight.

* fix(fuzz): point every harness at the API it claims to fuzz

Four of the five fuzz harnesses did not test what they name, and the
build could not say so because not one of them includes a project
header. Each declares its target itself:

  fuzz_recovery_protocol.c  extern int eos_recovery_parse_packet(...)
  fuzz_fw_update.c          extern int eos_fw_update_init(void)
                            extern int eos_fw_update_process_chunk(...)
                            extern int eos_fw_update_finalize(void)
  fuzz_bootctl.c            extern int eos_bootctl_parse(...)
  fuzz_image_verify.c       extern int eos_image_parse_header(const void *, size_t)

eos_recovery_parse_packet, eos_fw_update_init,
eos_fw_update_process_chunk and eos_bootctl_parse have never existed —
those names appear nowhere outside the harness that declares them, so
three of the five targets have never linked.

The other two are worse, because they do link:

  - eos_fw_update_finalize exists but takes (ctx, mode), not (void).
  - eos_image_parse_header takes (uint32_t addr, eos_image_header_t *),
    not (const void *, size_t). fuzz_image_verify has been passing a
    pointer where an address is expected and a length where an output
    struct is expected, on every input, for as long as it has run.

A local extern is why none of this was caught: it tells the compiler the
symbol exists with whatever shape the harness asserts, and the mismatch
survives to link time or past it.

All five now include the real header and drive the real entry points:

  fuzz_image_verify       parse_header -> verify_integrity, boot-path order
  fuzz_bootctl            bootctl_load, then trap if load() accepted a
                          block validate() rejects
  fuzz_fw_update          begin -> write in fuzz-chosen chunk widths ->
                          finalize or abort
  fuzz_recovery_protocol  write_in_range, trapping on any accepted write
                          that leaves the slot or wraps, checked in wider
                          types that cannot
  fuzz_crypto             already correct; switched to the header so it
                          stays that way

Adds tests/fuzz/fuzz_sim_flash.h: the three harnesses that reach flash
need board ops installed, and without them they fuzz a null op table.

That the compiler now checks this is not theoretical — writing this, it
rejected EOS_UPGRADE_MODE_TEST for EOS_UPGRADE_TEST immediately, which
is exactly the error class the externs were hiding.

Verified. libFuzzer is unavailable here (libclang_rt.fuzzer_osx.a not
found), so each harness was linked against eboot_core and driven by a
standalone seed generator under ASan+UBSan:

  all five compile -Wall -Wextra    5/5, 0 failures
  20,000 inputs each               100,000 total, no reports
  ctest                            21/21 passed

Third instance of this class across these repos, after eos#50
(fuzz_devicetree naming eos_dtb_parse) and eos#110 (fuzz_ota_header
naming eos_ota_parse_header). The common factor every time is a fuzz
target declaring its own subject.

Stacked on #94, without which include/eos_image.h does not compile.

* fix(fuzz): the recovery oracle was off by one, and CI proved it

The restored Fuzz Harness Build job on #101 ran these harnesses under
real libFuzzer for the first time and fuzz_recovery_protocol trapped
within seconds. That was my bug, not the code's.

The oracle computed the one-past-the-end address:

    end = base + offset + len;  if (end > 0xFFFFFFFF) trap;

The last byte a write of len bytes touches is base + offset + len - 1.
eos_recovery_write_in_range() checks exactly that:

    if ((uint32_t)len - 1u > UINT32_MAX - (base + offset)) reject;

So an accepted write whose final byte lands exactly on 0xFFFFFFFF --
base=0xFFFFFF00, offset=0, len=256 -- is legal, the function accepts it,
and the old oracle trapped on it. A harness that traps on valid input
reports the opposite of the truth.

Verified both directions:
  the boundary case, fed directly      -> no trap
  a fail-open stub accepting all input -> trap fires (exit 133)
  500,000 random inputs, ASan+UBSan    -> no reports

libFuzzer doing precisely what it is for -- driving inputs into the one
corner a hand-written oracle got wrong -- is the strongest argument yet
for #101's job. The guard could never have caught this; only execution
under a coverage-driven fuzzer did.
srpatcha added a commit that referenced this pull request Sep 8, 2026
* fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken

master (22d8f8b) does not compile. Two independent double-merges, both the
same shape: two PRs fixing adjacent things landed on stale bases, each was
green on its own branch, and the result was never rebuilt.

1. include/eos_image.h — #93 replaced reserved[30] with tlv_len (2) +
   tlv_hash[28], preserving every offset. #87 merged afterwards carrying
   asserts written against the older struct:

     error: no member named 'reserved' in 'eos_image_header_t'   (x2)

   #93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert
   was a duplicate; the width assert had no replacement and is restored as two
   asserts covering both halves of the same 30-byte span. No offset moves and
   the wire format is unchanged.

2. core/ed25519_verify.c — #86 and #57 both landed a subgroup guard, so the
   file carried two byte-identical point_is_identity() definitions:

     error: redefinition of 'point_is_identity'

   Only #57's public_key_is_valid_subgroup() is wired to the call site, so
   #86's key_has_prime_order() was dead. Kept the live function, folded #86's
   fuller rationale onto it, deleted the duplicate.

3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of
   test_ed25519_identity_key_forgery_rejected, main() calling it twice and two
   tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery
   referencing k_low_order[] and messages[] that the merge had dropped.

   While restoring the corpus, corrected it (review finding on #86): the array
   claimed to hold "the eight low-order point encodings" and held five. Every
   order here was computed rather than copied — decode y, recover x, add the
   point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8,
   8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8
   encodings. D9FF..FF was in the array and is not a low-order point at all —
   no x satisfies the curve equation for that y — so it moves to a separate
   k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1).

   tests_run was assigned a literal (11) in main() and never incremented,
   which is how the duplicate call and the two unregistered tests went
   unnoticed. The TEST macro now increments it, so the total cannot drift.

Verified:
  cmake -DEBLDR_BUILD_TESTS=ON on master   FAILS to build, 3 errors
  same with this commit                    builds clean
  ctest                                    21/21 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   21/21 PASS
  pytest tests/                            24 passed, 1 skipped
  test_ed25519                             14/14 PASS (was 11 claimed, 12 run)
  discrimination, with `public_key_is_valid_subgroup` disabled:
    test_ed25519_low_order_keys_rejected   FAILS, as it must
    test_ed25519_non_canonical_...         still PASSES — those are refused by
      unpackneg() on canonicality, a different mechanism, which is the reason
      they are held in a separate array rather than counted among the eight.

* fix(fdt): bound a device tree parser that was never compiled

core/fdt_loader.c is in no source list, so it has never been built. The
header offers it to callers, rtos_boot.c is named as the consumer, and
nothing catches what is in it -- not the compiler, not ctest, not the
sanitizer job.

What is in it is a parser for a blob that comes out of flash, whose every
header field it uses as an offset or a length without checking any of
them. eos_fdt_validate() looks at magic and version only, so it accepts
a blob whose off_dt_struct points anywhere:

    hdr.totalsize     = 40      (the header alone)
    hdr.off_dt_struct = 0x100000
    eos_fdt_validate(blob) -> 0
    eos_fdt_get_prop(...)  -> AddressSanitizer: BUS, READ at
                              fdt_loader.c:25 in fdt_read_u32

Five more, all reachable the same way:

* the tag read at the top of the loop takes 4 bytes where the loop
  condition guarantees 1;
* strlen() on a node name reads until it finds a zero, which for a name
  running to the end of the block is past it;
* nameoff indexes the strings block unchecked, so strcmp() reads from an
  arbitrary address;
* a property len is clamped to the caller's buffer before the memcpy,
  which bounds the write but not the read -- an oversized len copies
  whatever follows the blob out to the caller;
* FDT_END_NODE decrements depth with no floor.

Bound them. validate() is the gate every path goes through, so the block
offsets are checked against totalsize there, and get_prop() calls it
before trusting the header. Inside the loop each read is checked for the
width it takes, the name and property-name scans are bounded by memchr
within their blocks, and the padded advances are re-checked for overrun.

Adds the file to eboot_core and tests/unit/test_fdt_loader.c to ctest:
ten cases, one well-formed tree that must still parse and nine malformed
ones. Against the unfixed parser the suite fails on the third; with the
bounds in place the whole suite is 20/20, and 20/20 under EBLDR_SANITIZE.

Not fixed here, because it is a behaviour change rather than a safety
one: node paths below the root do not resolve. _get_prop derives the
depth to match from a slash count, so "/chosen" looks for depth 1, which
is the root -- "chosen" is at depth 2. Only "/" resolves today.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(fdt): bound the parser against the caller's length, not the blob's claim

Follow-up to the review on #84. Three findings from it, all in files this
branch already owns.

Finding 1 (High) -- every bound in eos_fdt_validate() was expressed relative
to hdr->totalsize, which is read out of the same untrusted blob. Checking the
block offsets against it proves the header is internally consistent and says
nothing about how many bytes are really mapped: a 40-byte buffer declaring
totalsize = 0x100000 with agreeing offsets passed every check, and
eos_fdt_get_prop() then walked a megabyte past the allocation. The original
reproducer (an inflated *offset* against an honest totalsize) was rejected;
its mirror image was not.

Adds the length-carrying entry points eos_fdt_validate_sized() and
eos_fdt_get_prop_sized(), which take the bytes the caller actually owns and
check totalsize against that before anything else. The existing two-argument
forms stay as wrappers that trust the blob's own totalsize, documented in the
header as a warrant the caller has to make good; eos_fdt_load() now passes
max_size, which it had all along.

Finding 2 (Medium) -- a property larger than the caller's buffer was copied
short and reported as success, so a clipped value was indistinguishable from
a complete one. In a boot path this reads bootargs. Now returns -7 and sets
*buf_len to the full length so the caller can size a retry.

Finding 3 (Medium) -- adds tests/fuzz/fuzz_fdt.c alongside the five existing
harnesses. .ai/security.md names device tree among the parsers that get fuzz
coverage rather than unit tests alone. It drives the sized entry points and
passes the real size, which is what makes the harness meaningful -- the
unsized forms would let a fuzzer authorise its own out-of-bounds read.

Finding 6 (Low) -- drops the unreachable `total < sizeof(fdt_header_t)` check
in eos_fdt_load(); validate() already rejects that blob, and the comment
above it described a bound it was not applying.

Header now documents the full -1..-8 return code table.

Verified:
  cmake -DEBLDR_BUILD_TESTS=ON, ctest              21/21 PASS
  same under -DEBLDR_SANITIZE=ON (ASan+UBSan)      21/21 PASS
  test_fdt_loader                                  14 tests PASS (was 10)
  new tests against the pre-fix parser             both FAIL as they should
    - inflated totalsize accepted at the sized entry point
    - truncating call returned 0 instead of -7
  grep for eos_fdt_get_prop / eos_fdt_validate     no callers outside the
    parser and its tests, so the -7 change breaks nothing today

Refs #84

* fix(build): drop the duplicate core/fdt_loader.c registration

master gained core/fdt_loader.c in eboot_core while this branch was open, so
after the rebase it appeared twice and
tests/unit/test_cmake_core_sources.py::test_core_sources_are_registered_once
failed:

    AssertionError: ['core/fdt_loader.c'] is not false :
    duplicate eboot_core sources: ['core/fdt_loader.c']

Kept master's entry, dropped the one this branch added. That guard is the
same shape as the one this stack adds for toolchain specs -- a build-file
check that catches the class rather than the instance -- and it did its job.

Verified: ctest 22/22 PASS, pytest 24 passed 1 skipped.

Refs #84

* fix(fdt): one length-carrying API, and a CI job that builds the fuzz harnesses

Answers the second review on #84.

Finding 2 (Medium) -- eos_fdt_validate() and eos_fdt_get_prop() kept an
out-of-bounds read by design. Both dereferenced hdr->totalsize before any
length was known and handed that attacker-controlled value to the _sized form
as its bound, so calling either on a short buffer read past it before a single
check had run. The header called that a caller warrant; a warrant is not a
check, and it is the exact bug this PR exists to fix. Neither had an in-tree
caller.

Removed rather than documented. There is now one form of each entry point and
it always takes the length:

    int eos_fdt_validate(const void *fdt_blob, uint32_t avail);
    int eos_fdt_get_prop(const void *fdt, uint32_t fdt_len, ...);

An exported unsafe twin in a TCB header is a future boot-path caller
reintroducing the bug with no compiler complaint, which is worth more than the
convenience of a one-argument call. eos_fdt_load() already passed max_size.
The test that pinned the wrapper's behaviour is gone with the wrapper, and
get_prop_sized_exact() collapsed into get_prop_exact() since they became the
same function.

Finding 3 (Medium) -- tests/fuzz/ was built by nothing. EBLDR_BUILD_FUZZ
defaults OFF and no job set it, so the harness added here joined five others
that no CI job compiles. A harness that is never built cannot fail to build,
which is how fuzz_devicetree came to declare a function that did not exist and
sit there unnoticed (eos#50).

Adds a `fuzz-build` job: configure with clang, build every harness, and run
each for five seconds over its own generated inputs. That is not a campaign --
it is enough to catch a harness that no longer compiles or crashes at once,
which is the failure this repo has actually had. Note it needs
EBLDR_BUILD_TESTS=ON as well: tests/fuzz/ is added from tests/CMakeLists.txt,
so EBLDR_BUILD_FUZZ alone configures cleanly and builds no harness at all --
the job would have passed having compiled nothing. Found that locally before
writing the job, not after.

  NOT RUN, and this is the honest limit: this host has no libFuzzer runtime
  (libclang_rt.fuzzer_osx.a is absent from the Xcode toolchain), so the link
  step cannot be reproduced here for any harness, old or new. What I verified
  is that CMake configures with both flags and reports all six targets --
  "Fuzz targets: fuzz_image_verify, fuzz_recovery_protocol, fuzz_fw_update,
  fuzz_crypto, fuzz_bootctl, fuzz_fdt" -- and that fuzz_fdt.c passes
  `cc -fsyntax-only`. The link and the smoke run are CI's to show, and this
  job is what makes them visible.

  Coordination note: #90 adds a `CI Gate` whose needs list is
  [test, build-arm, static-analysis]. Whichever of #84 and #90 lands second
  must add fuzz-build to that list -- and #90's own
  test_gate_covers_every_job_that_runs_on_a_pull_request fails loudly if it is
  not, which is the guard working rather than a trap.

Finding 1 (High) is a PR-body correction, made there: the diff carries #94's
master repair because this branch is stacked on it, and the body described
only the FDT parser.

Verified:
  ctest                                    22/22 PASS
  pytest tests/                            38 passed
  test_fdt_loader                          13 tests PASS
  grep for a length-free entry point       none remains in the header

Refs #84

* ci: move the fuzz-harness build to its own PR

The fuzz-build job added in the previous commit does its job -- it is red, on
harnesses this PR did not write:

    undefined reference to `eos_fw_update_init'
    undefined reference to `eos_fw_update_process_chunk'
    undefined reference to `eos_recovery_parse_packet'

None of those exist anywhere in the tree. tests/fuzz/fuzz_fw_update.c and
fuzz_recovery_protocol.c declare an API that was never written, exactly as
eos#50's fuzz_devicetree declared eos_dtb_parse(). They have compiled forever
because nothing ever linked them.

That is a real finding and it deserves a fix, not a red tick on an unrelated
PR. The job and the repairs move to their own change; this PR keeps
tests/fuzz/fuzz_fdt.c, which is the harness it is responsible for.

Refs #84

* fix(fdt): read the header the way the struct block is read, and stop resynchronising on garbage

Answers the third review on #84.

Finding 3 (Low, and the one that matters on hardware) -- the header was read
through a cast fdt_header_t* and direct member loads, while every struct-block
read goes through the fdt_read_u32() memcpy helper precisely because the blob
may be unaligned. Fuzz input, a buffer inside a larger message, a copy at an
odd offset: nothing promises 4-byte alignment, and on a strict-alignment
cross target a direct member load is the same fault class this parser exists
to avoid. The unit suite could never catch it -- blob_t.bytes happens to be
aligned.

All header fields now go through fdt_hdr_u32() (memcpy at offsetof), one rule
for the whole blob, in validate(), load() and get_prop(). FUZZ_FLAGS gains
`undefined` so the fuzz job checks alignment too.

Finding 4 (Low) -- `default: break;` resynchronised on unrecognised tags: the
walk skipped 4 bytes and treated whatever followed as the next token, so a
struct block of arbitrary bytes parsed to a clean "not found". Bounded, but a
TCB parser that walks garbage to completion is accepting input it does not
understand. FDT_NOP -- the one legal unknown, padding the spec allows between
tokens -- is now named in the header and passes; anything else returns -6.

Findings 1, 2 and the -8 line: the fuzz-build job's single home is #101
(nothing added here; the harness is inert until that job lands and then
compiled by it -- said on the thread, not just here), and the header no longer
documents return code -8, which nothing at this head returns. #85 adds the -8
return and re-documents it together with the FDT_MAX_PATH_DEPTH move.

Verified:
  ctest                                    22/22 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   22/22 PASS
  pytest tests/                            38 passed
  test_fdt_loader                          16 tests PASS (was 13)
  discrimination, each fix in isolation:
    default: break restored     -> test_a_garbage_tag_is_refused_not_skipped
                                   FAILS (expects -6, gets "not found")
    direct member load restored -> under UBSan the new unaligned-blob test
                                   reports "load of misaligned address
                                   0x...671 for type 'const uint32_t'" at the
                                   exact line -- and cannot fire on the fixed
                                   code, which runs the same test clean
  NOP counter-check: interleaved FDT_NOP tokens still resolve, so the
  stricter default does not reject real trees.

Refs #84

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Srikanth Patchava <srikanth.patchava@outlook.com>
srpatcha pushed a commit that referenced this pull request Sep 8, 2026
…ul boot (#82)

* fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken

master (22d8f8b) does not compile. Two independent double-merges, both the
same shape: two PRs fixing adjacent things landed on stale bases, each was
green on its own branch, and the result was never rebuilt.

1. include/eos_image.h — #93 replaced reserved[30] with tlv_len (2) +
   tlv_hash[28], preserving every offset. #87 merged afterwards carrying
   asserts written against the older struct:

     error: no member named 'reserved' in 'eos_image_header_t'   (x2)

   #93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert
   was a duplicate; the width assert had no replacement and is restored as two
   asserts covering both halves of the same 30-byte span. No offset moves and
   the wire format is unchanged.

2. core/ed25519_verify.c — #86 and #57 both landed a subgroup guard, so the
   file carried two byte-identical point_is_identity() definitions:

     error: redefinition of 'point_is_identity'

   Only #57's public_key_is_valid_subgroup() is wired to the call site, so
   #86's key_has_prime_order() was dead. Kept the live function, folded #86's
   fuller rationale onto it, deleted the duplicate.

3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of
   test_ed25519_identity_key_forgery_rejected, main() calling it twice and two
   tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery
   referencing k_low_order[] and messages[] that the merge had dropped.

   While restoring the corpus, corrected it (review finding on #86): the array
   claimed to hold "the eight low-order point encodings" and held five. Every
   order here was computed rather than copied — decode y, recover x, add the
   point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8,
   8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8
   encodings. D9FF..FF was in the array and is not a low-order point at all —
   no x satisfies the curve equation for that y — so it moves to a separate
   k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1).

   tests_run was assigned a literal (11) in main() and never incremented,
   which is how the duplicate call and the two unregistered tests went
   unnoticed. The TEST macro now increments it, so the total cannot drift.

Verified:
  cmake -DEBLDR_BUILD_TESTS=ON on master   FAILS to build, 3 errors
  same with this commit                    builds clean
  ctest                                    21/21 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   21/21 PASS
  pytest tests/                            24 passed, 1 skipped
  test_ed25519                             14/14 PASS (was 11 claimed, 12 run)
  discrimination, with `public_key_is_valid_subgroup` disabled:
    test_ed25519_low_order_keys_rejected   FAILS, as it must
    test_ed25519_non_canonical_...         still PASSES — those are refused by
      unpackneg() on canonicality, a different mechanism, which is the reason
      they are held in a separate array rather than counted among the eight.

* fix(secure-boot): a debug lock that failed must not report a successful boot

cfg.lock_debug asks for SWD/JTAG to be closed before the verified image runs.
Step 7 of eos_secure_boot() honoured that request like this:

    if (cfg->lock_debug) {
        eos_secure_boot_lock_debug();
    }

    /* ---- Step 8: Record successful attestation ---- */
    attest_record(2, hdr.image_version, hdr.hash, NULL, EOS_SBOOT_OK);
    return EOS_SBOOT_OK;

eos_secure_boot_lock_debug() returned void and discarded the result of the OTP
write that actually blows the fuse. So when the write failed, boot continued,
attestation recorded EOS_SBOOT_OK, and the device ran the image with its debug
port open -- the exact condition the policy existed to prevent, reported as a
clean secure boot.

The interesting case is not a flaky fuse. eos_hal_otp_write() returns
EOS_ERR_NOT_SUPPORTED when the board provides no otp_write hook at all, so on
every such board `lock_debug: true` was silently a no-op. That is the default
configuration, not an edge case.

eos_secure_boot_lock_debug() now returns int, and a caller that asked for the
lock and did not get it fails with EOS_SBOOT_ERR_POLICY -- a code that already
existed for exactly this ("Boot policy violation") -- with the failure recorded
in the attestation log rather than a success.

Why this was never observable: core/secure_boot.c is not in CMakeLists.txt.
The module has never been compiled, so this path could not run and could not be
tested. Added the one line that builds it -- the same line #72 adds, written
identically so whichever lands first leaves the other a trivial rebase.

tests/unit/test_secure_boot_policy.c covers the three outcomes: the fuse
written, the write failing, and a board with no otp_write. Kept in its own file
so it does not collide with the test_secure_boot.c #72 introduces.

Against master the new test does not compile -- `invalid operands to binary
expression ('void' and 'int')` -- because there is no result to check. That is
the defect stated as a compile error.

Verified on this branch: build clean, ctest 20/20, pytest 30 passed.

Stacked on #77 (master's test suite does not compile without it).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(secure-boot): test the policy the PR changed, not only the helper it calls

Answers finding 1 (Medium) from the review on #82.

All three tests called eos_secure_boot_lock_debug() directly and asserted its
return value. None drove eos_secure_boot() with cfg.lock_debug = true, so the
step-7 early return this PR adds -- the attest_record + return
EOS_SBOOT_ERR_POLICY -- never executed under test. The file's docblock says
"the debug-lock policy must be enforced, not merely attempted"; what it
covered was the attempted half.

Adds three end-to-end cases through eos_secure_boot():

  - lock_debug: true on a board with no otp_write  -> EOS_SBOOT_ERR_POLICY
  - the same image with lock_debug: false          -> EOS_SBOOT_OK, and the
    recorded entry point is the header's, which is the counter-check: without
    it the first test would also pass if steps 1-6 were failing for an
    unrelated reason and never reaching step 7
  - lock_debug: true with a working fuse           -> EOS_SBOOT_OK, one write

Reaching step 7 needs an image that clears steps 1, 2 and 5, so the fixture
gains a simulated flash and stages an unsigned, unencrypted image whose
SHA-256 matches. EOS_IMG_FLAG_HASH_SHA256 is load-bearing there: without it
verify_integrity takes the CRC32 branch, reads a CRC out of hash[], and
step 2 fails before the policy step is ever reached.

Finding 4 (Low), the tests/CMakeLists.txt anchor shared with #80, is resolved
on #80's side -- its block moved to the end of the registrations, so this one
keeps its position and the two no longer collide.

Verified:
  ctest                                  22/22 PASS
  test_secure_boot_policy                6/6 PASS
  discrimination: with step 7 reverted to `(void)eos_secure_boot_lock_debug();`
    test_secure_boot_refuses_when_the_debug_lock_cannot_be_taken  FAILS
    the three original helper tests                               still PASS
  which is the gap the finding described, reproduced.

Refs #82

* fix(secure-boot): step 4 must not report success for a check it never makes

Answers the second review on #82.

Finding 1 (High) -- step 4, "Verify signing key against OTP root-of-trust",
was the same fail-open this PR removes from step 7, two steps earlier. An
`eos_hal_otp_read()` failure was discarded and boot continued; and when the
read succeeded and the anchor was provisioned, the body of the `if` was
`/* In a full implementation, extract key hash from TLV and compare */`. So a
device whose root of trust *is* provisioned booted an image signed by any key
the image carried, and step 8 recorded EOS_SBOOT_OK.

Implementing the TLV comparison is out of scope, as the review says. The two
things that are in scope are done: a non-EOS_OK otp_read now fails
EOS_SBOOT_ERR_SIGNATURE, and a provisioned anchor that nothing compares
against refuses the boot rather than proceeding. An unprovisioned board
(all-zero anchor) is deliberately unchanged -- there is nothing to check
against, and refusing would brick every board that has not been provisioned.
The comment now states plainly that the step is planned rather than
implemented, which §8.1 asks for and a comment inside an `if` was not.

  No test for those two refusals, deliberately, and the file says why rather
  than leaving it to be discovered. Reaching step 4 requires passing step 3 --
  a real Ed25519 signature checked against the keystore. I wrote the obvious
  test first and it was worthless: with require_signature = true and an
  unsigned fixture the boot fails at step 3 and returns the same
  EOS_SBOOT_ERR_SIGNATURE step 4 returns, so it passed against the unfixed
  code too. I confirmed that by reverting step 4 and watching it still pass.
  A test that cannot fail is worse than none. What is there instead is the
  counter-check that the change does not refuse a boot it should allow.

  The fixture is buildable -- the keystore ships RFC 8032 TEST 1's public key
  and the matching private key is in the RFC -- but that machinery is #88's
  (tools/gen_signed_image_fixture.py). Worth doing once #88 lands.

Finding 3 (Low) -- `TEST()` did not increment `tests_run` and `main()`
hardcoded `tests_run = 6`, so a test added to the file but not wired into
`main()` would have been skipped with a zero exit. #94 removed exactly this
from tests/unit/test_ed25519.c, where a hardcoded 11 was masking two uncalled
tests. Same fix here.

Finding 4 (Low) -- the Valgrind `foreach` is hand-maintained and missed 6 of
22 registered tests. Added test_secure_boot_policy, test_fdt_loader and
test_fw_decrypt. The list being hand-maintained at all is the real defect and
is not fixed here -- it is the same class as the hardcoded count, one level
up.

Finding 2 (Medium), that eos_secure_boot() has no production caller, stands
and is not addressed here; finding 1 makes it sharper rather than resolving
it. Finding 5 (Low) is a PR-body correction.

Verified:
  ctest                          22/22 PASS
  test_secure_boot_policy        7/7 PASS
  step 7 discrimination, still: reverting the step-7 branch fails
    test_secure_boot_refuses_when_the_debug_lock_cannot_be_taken

Refs #82

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
srpatcha added a commit that referenced this pull request Sep 8, 2026
…ing them (#95)

* fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken

master (22d8f8b) does not compile. Two independent double-merges, both the
same shape: two PRs fixing adjacent things landed on stale bases, each was
green on its own branch, and the result was never rebuilt.

1. include/eos_image.h — #93 replaced reserved[30] with tlv_len (2) +
   tlv_hash[28], preserving every offset. #87 merged afterwards carrying
   asserts written against the older struct:

     error: no member named 'reserved' in 'eos_image_header_t'   (x2)

   #93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert
   was a duplicate; the width assert had no replacement and is restored as two
   asserts covering both halves of the same 30-byte span. No offset moves and
   the wire format is unchanged.

2. core/ed25519_verify.c — #86 and #57 both landed a subgroup guard, so the
   file carried two byte-identical point_is_identity() definitions:

     error: redefinition of 'point_is_identity'

   Only #57's public_key_is_valid_subgroup() is wired to the call site, so
   #86's key_has_prime_order() was dead. Kept the live function, folded #86's
   fuller rationale onto it, deleted the duplicate.

3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of
   test_ed25519_identity_key_forgery_rejected, main() calling it twice and two
   tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery
   referencing k_low_order[] and messages[] that the merge had dropped.

   While restoring the corpus, corrected it (review finding on #86): the array
   claimed to hold "the eight low-order point encodings" and held five. Every
   order here was computed rather than copied — decode y, recover x, add the
   point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8,
   8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8
   encodings. D9FF..FF was in the array and is not a low-order point at all —
   no x satisfies the curve equation for that y — so it moves to a separate
   k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1).

   tests_run was assigned a literal (11) in main() and never incremented,
   which is how the duplicate call and the two unregistered tests went
   unnoticed. The TEST macro now increments it, so the total cannot drift.

Verified:
  cmake -DEBLDR_BUILD_TESTS=ON on master   FAILS to build, 3 errors
  same with this commit                    builds clean
  ctest                                    21/21 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   21/21 PASS
  pytest tests/                            24 passed, 1 skipped
  test_ed25519                             14/14 PASS (was 11 claimed, 12 run)
  discrimination, with `public_key_is_valid_subgroup` disabled:
    test_ed25519_low_order_keys_rejected   FAILS, as it must
    test_ed25519_non_canonical_...         still PASSES — those are refused by
      unpackneg() on canonicality, a different mechanism, which is the reason
      they are held in a separate array rather than counted among the eight.

* test: derive the suite totals and the Valgrind list instead of restating them

Two findings arrived one after another in review, on different PRs, with the
same shape underneath: a number or a list that describes what the tests do,
written out separately from the thing it describes, with nothing checking the
two agree. Fixing them one instance at a time was going to keep producing the
same finding, so this fixes the class and adds the guard.

**1. Suites stated a total instead of counting one.** The `TEST()` macro
incremented `tests_passed`; `main()` assigned `tests_run = <literal>`. A test
defined but never wired into `main()` was skipped with a zero exit, and the
suite still reported "N/N passed" for an N that was a claim.

  18 of 21 suites carried it. It was not theoretical:

  - `test_ed25519.c` had a hardcoded 11 that masked one test called twice and
    two never called at all (repaired in #94, which is where this started).
  - `test_tlv_auth.c` assigns `tests_run` twice in one function -- 8, then 7.
    The stale 8 survives only because the later assignment wins. Its suite
    reports 7/7 today by luck.
  - `test_secure_boot.c` counted nothing and ended `return 0`, so a suite that
    ran none of its cases still reported success. The ASSERT macro exits on
    failure, so that return could only ever have signalled the one case it
    ignored.

  `TEST()` now increments `tests_run` where it invokes the test, every literal
  assignment and every `%d/<literal>` in a summary is gone, and
  `test_secure_boot.c` compares and returns accordingly.

**2. The Valgrind list named its suites again by hand**, and had drifted to 17
of 21 -- `test_ecc`, `test_rollback`, `test_secure_boot` and `test_storage`
got no memory-safety run, and nothing failed when a name was forgotten.
Each `add_test()` now appends to `EBLDR_UNIT_TESTS` and the `foreach` iterates
that, so a suite added without touching the block still gets a Valgrind
target. Confirmed with a stub `valgrind` on PATH: 21 `valgrind_*` tests are
generated, up from 17.

**3. tests/unit/test_suite_bookkeeping.py** keeps both from returning. Four
checks, no C toolchain needed: no suite hardcodes its own total; every
`TEST()` macro counts the test it runs; every defined test is actually called;
and the Valgrind list is derived rather than repeated. Three suites with no
`TEST()` macro are listed in `NO_TEST_MACRO` with a reason each.

Verified:
  ctest                                    21/21 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   21/21 PASS
  pytest tests/                            42 passed
  valgrind targets, with a stub on PATH    21 (was 17)
  every suite's reported total now equals what it ran -- e.g. test_bootctl
    12/12, test_tlv_auth 7/7, test_secure_boot 4/4

  Each of the four guards was probed against the defect it is written for:
    reintroduce `tests_run = 12`            -> test_bootctl.c: tests_run = 12
    remove `tests_run++` from the macro     -> FAIL
    stop calling a defined TEST()           -> FAIL
    drop a suite from EBLDR_UNIT_TESTS      -> FAIL
  and all four pass again on restore.

Refs #94, #80, #82

---------

Co-authored-by: Srikanth Patchava <srikanth.patchava@outlook.com>
srpatcha pushed a commit that referenced this pull request Sep 8, 2026
…ped (#81)

* fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken

master (22d8f8b) does not compile. Two independent double-merges, both the
same shape: two PRs fixing adjacent things landed on stale bases, each was
green on its own branch, and the result was never rebuilt.

1. include/eos_image.h — #93 replaced reserved[30] with tlv_len (2) +
   tlv_hash[28], preserving every offset. #87 merged afterwards carrying
   asserts written against the older struct:

     error: no member named 'reserved' in 'eos_image_header_t'   (x2)

   #93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert
   was a duplicate; the width assert had no replacement and is restored as two
   asserts covering both halves of the same 30-byte span. No offset moves and
   the wire format is unchanged.

2. core/ed25519_verify.c — #86 and #57 both landed a subgroup guard, so the
   file carried two byte-identical point_is_identity() definitions:

     error: redefinition of 'point_is_identity'

   Only #57's public_key_is_valid_subgroup() is wired to the call site, so
   #86's key_has_prime_order() was dead. Kept the live function, folded #86's
   fuller rationale onto it, deleted the duplicate.

3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of
   test_ed25519_identity_key_forgery_rejected, main() calling it twice and two
   tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery
   referencing k_low_order[] and messages[] that the merge had dropped.

   While restoring the corpus, corrected it (review finding on #86): the array
   claimed to hold "the eight low-order point encodings" and held five. Every
   order here was computed rather than copied — decode y, recover x, add the
   point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8,
   8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8
   encodings. D9FF..FF was in the array and is not a low-order point at all —
   no x satisfies the curve equation for that y — so it moves to a separate
   k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1).

   tests_run was assigned a literal (11) in main() and never incremented,
   which is how the duplicate call and the two unregistered tests went
   unnoticed. The TEST macro now increments it, so the total cannot drift.

Verified:
  cmake -DEBLDR_BUILD_TESTS=ON on master   FAILS to build, 3 errors
  same with this commit                    builds clean
  ctest                                    21/21 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   21/21 PASS
  pytest tests/                            24 passed, 1 skipped
  test_ed25519                             14/14 PASS (was 11 claimed, 12 run)
  discrimination, with `public_key_is_valid_subgroup` disabled:
    test_ed25519_low_order_keys_rejected   FAILS, as it must
    test_ed25519_non_canonical_...         still PASSES — those are refused by
      unpackneg() on canonicality, a different mechanism, which is the reason
      they are held in a separate array rather than counted among the eight.

* ci: make the EoSim workflow able to fail for the reasons it claims to check

Answers the review on #81. The install fix was right; the workflow it revives
could not have failed for most of what it says it verifies.

Finding 1 (Medium) -- the step named "Validate all platform configs" ran
`eosim list && eosim doctor` with no platforms/ copy in that job. `eosim list`
prints "Available platforms (0)" without it and exits 0, so the step passed
having validated nothing -- and the same shape was in windows-sanity and
macos-sanity. All three now copy platforms/ from the pinned checkout and
assert a non-zero count via discover_platforms(), because the exit code is
exactly what cannot be trusted here.

Finding 2 (Medium) -- nested-guest-install cloned EoSim a second time with no
--branch for its platforms/ copy, so the package came from
v${EOSIM_VERSION} while the platform data came from whatever the default
branch pointed at that morning. Now copies from the same pinned checkout; the
second clone is gone.

Finding 3 (Medium) -- `eosim --version` was printed and never asserted, while
the tag and the package's declared version disagree upstream (v1.5.0 ships
"eosim, version 2.0.0"). Pinning to a tag therefore does not pin what the name
suggests, and nothing would have noticed if the tag moved. Now asserted. The
mismatch itself is EoSim's bug and is raised there rather than worked around
here.

Finding 4 (Medium) -- sanity-gate failed only on install-validate and then
printed "All EoSim sanity checks passed", which it would do with the other
four jobs red. Replaced with the toJSON(needs) + jq body from ci.yml, which
cannot fall out of step with `needs:`. #90 adds a test that enforces this
across every gate in the repository; this gate passes it.

Verified:
  yaml.safe_load of eosim-sanity.yml     parses, 6 jobs
  the gate now iterates toJSON(needs), no longer branches on
    install-validate alone, and no longer prints an "all passed" claim
  #90's test_no_aggregating_gate_ignores_part_of_its_needs, run against this
    workflow: eosim-sanity.yml is not among its offenders
  pytest tests/                          38 passed
  ctest                                  21/21 PASS

  NOT RUN: the workflow itself. It is `on: schedule` + `workflow_dispatch`
  only, so none of this PR's checks execute it -- which is finding 5, and it
  is the one piece of evidence this PR cannot produce from a fork branch
  without a maintainer dispatching it. The install sequence was verified
  locally end to end (clone -> pip install -> eosim --version 2.0.0 ->
  doctor -> run am62x --headless PASSED); the assertions added here are not
  covered by that and remain unexecuted.

Refs #81
srpatcha pushed a commit that referenced this pull request Sep 8, 2026
…ing GCM (#80)

* fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken

master (22d8f8b) does not compile. Two independent double-merges, both the
same shape: two PRs fixing adjacent things landed on stale bases, each was
green on its own branch, and the result was never rebuilt.

1. include/eos_image.h — #93 replaced reserved[30] with tlv_len (2) +
   tlv_hash[28], preserving every offset. #87 merged afterwards carrying
   asserts written against the older struct:

     error: no member named 'reserved' in 'eos_image_header_t'   (x2)

   #93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert
   was a duplicate; the width assert had no replacement and is restored as two
   asserts covering both halves of the same 30-byte span. No offset moves and
   the wire format is unchanged.

2. core/ed25519_verify.c — #86 and #57 both landed a subgroup guard, so the
   file carried two byte-identical point_is_identity() definitions:

     error: redefinition of 'point_is_identity'

   Only #57's public_key_is_valid_subgroup() is wired to the call site, so
   #86's key_has_prime_order() was dead. Kept the live function, folded #86's
   fuller rationale onto it, deleted the duplicate.

3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of
   test_ed25519_identity_key_forgery_rejected, main() calling it twice and two
   tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery
   referencing k_low_order[] and messages[] that the merge had dropped.

   While restoring the corpus, corrected it (review finding on #86): the array
   claimed to hold "the eight low-order point encodings" and held five. Every
   order here was computed rather than copied — decode y, recover x, add the
   point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8,
   8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8
   encodings. D9FF..FF was in the array and is not a low-order point at all —
   no x satisfies the curve equation for that y — so it moves to a separate
   k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1).

   tests_run was assigned a literal (11) in main() and never incremented,
   which is how the duplicate call and the two unregistered tests went
   unnoticed. The TEST macro now increments it, so the total cannot drift.

Verified:
  cmake -DEBLDR_BUILD_TESTS=ON on master   FAILS to build, 3 errors
  same with this commit                    builds clean
  ctest                                    21/21 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   21/21 PASS
  pytest tests/                            24 passed, 1 skipped
  test_ed25519                             14/14 PASS (was 11 claimed, 12 run)
  discrimination, with `public_key_is_valid_subgroup` disabled:
    test_ed25519_low_order_keys_rejected   FAILS, as it must
    test_ed25519_non_canonical_...         still PASSES — those are refused by
      unpackneg() on canonicality, a different mechanism, which is the reason
      they are held in a separate array rather than counted among the eight.

* fix(fw_decrypt): drop a HW-crypto shortcut that cannot express streaming GCM

core/fw_decrypt.c is a hand-written AES-256-GCM sitting in the secure boot
path with no tests. It has an opt-in fast path:

    if (ops && ops->hw_aes_decrypt) {
        int rc = ops->hw_aes_decrypt(ctx->key, EOS_AES_KEY_SIZE,
                                     ctx->iv, data, data, len);
        if (rc == EOS_OK) { ctx->bytes_processed += len; return EOS_OK; }
    }

The hook is (key, key_len, iv, in, out, len). That signature cannot carry
streaming GCM, and taking the path broke decryption two ways:

1. No counter position. ctx->iv is passed unchanged on every call, so a second
   chunk restarts the CTR keystream at block 0 and is decrypted against the
   same keystream as the first. Reusing a CTR keystream across two plaintexts
   is the one thing the mode must never do.

2. No GHASH. The hook returns plaintext only, so ctx->ghash_acc is never fed.
   eos_fw_decrypt_final() computes the tag over an empty accumulator and
   rejects the image -- a board with an AES engine could not install a
   correctly encrypted firmware update at all.

(2) is why this was never noticed: it fails closed, and no board in-tree
implements the hook yet. (1) is why it cannot be patched by also feeding
GHASH: the plaintext would still be wrong past the first chunk. Re-enabling
needs a hook that takes a block offset and either exposes GHASH state or does
the whole GCM operation including the tag. Removed, with that written down
where the next person will look.

Also adds tests/unit/test_fw_decrypt.c -- the first tests this file has had.
Vectors come from an independent implementation (Python cryptography, i.e.
OpenSSL) rather than from this code, so they pin behaviour rather than
recording it:

  - whole blocks, and a 20-byte payload with a partial trailing block
  - the same ciphertext split [32] / [16,16] / [10,22] / [1,31] / [5,15]:
    GCM is a stream, so the result must not depend on how the caller sliced it
  - a board advertising a working AES engine must reach the same answer as one
    without -- this is the case that fails on master
  - every single-bit flip in the tag (128 of them), and in each ciphertext
    byte, must be rejected
  - unprovisioned/unreadable OTP keys and uninitialised contexts are refused

Verified: 8/8 pass with this change; against the unmodified fw_decrypt.c the
suite fails on
test_board_with_aes_engine_still_accepts_a_genuine_image, the assertion it
exists to make. The software GCM itself is correct -- I checked it against the
reference vectors before changing anything, including every chunk split above.

Note: the full test build on master is currently broken by test_image_verify.c
(fixed in #77), so this target was built directly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(fw_decrypt): answer the review — stale doc, orphaned hook, anchor clash

Three findings from the review on #80, all in files this branch already owns.

Finding 2 (Medium) -- core/fw_decrypt.c's file header still read "Falls back
to HAL hw_aes_decrypt if available." The diff started at line 192, so that
line survived and directly contradicted the twenty-line rationale this PR
installs below it. A self-contradictory file is worse than the original.

Finding 1 (Medium) -- after the removal, hw_aes_decrypt has zero call sites
while eos_hal.h still advertises it under "software fallback used if NULL",
which is now false in the other direction: a board author who implements the
hook gets nothing, silently, with no diagnostic. That is the same class of
failure this PR is fixing. Kept the member rather than deleting it -- removing
it from a public struct is an ABI change for out-of-tree boards, and the hook
is worth having once it can express the operation -- and documented it as
reserved and currently unconsumed, with the reason and with what a
streaming-capable replacement would need.

Finding 3 (Low) -- this PR and #82 both inserted their add_executable/add_test
triple immediately after add_test(NAME test_keystore ...), so whichever landed
second would have conflicted for no reason but placement. Re-anchored this
one to the end of the registrations, with a comment saying why.

Also rebased: this branch was 7 commits behind and its diff against current
master would have reverted the test_recovery link-line fix. It is now stacked
on #94, which repairs master -- without that, every PR that builds the test
suite is red on include/eos_image.h and core/ed25519_verify.c.

Verified:
  cmake --build (EBLDR_BUILD_TESTS=ON)   clean
  ctest                                  22/22 PASS
  test_fw_decrypt                        8/8 PASS
  git grep hw_aes_decrypt                header + this file's comment only

Refs #80

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
srpatcha pushed a commit that referenced this pull request Sep 8, 2026
…ugs (#101)

* fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken

master (22d8f8b) does not compile. Two independent double-merges, both the
same shape: two PRs fixing adjacent things landed on stale bases, each was
green on its own branch, and the result was never rebuilt.

1. include/eos_image.h — #93 replaced reserved[30] with tlv_len (2) +
   tlv_hash[28], preserving every offset. #87 merged afterwards carrying
   asserts written against the older struct:

     error: no member named 'reserved' in 'eos_image_header_t'   (x2)

   #93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert
   was a duplicate; the width assert had no replacement and is restored as two
   asserts covering both halves of the same 30-byte span. No offset moves and
   the wire format is unchanged.

2. core/ed25519_verify.c — #86 and #57 both landed a subgroup guard, so the
   file carried two byte-identical point_is_identity() definitions:

     error: redefinition of 'point_is_identity'

   Only #57's public_key_is_valid_subgroup() is wired to the call site, so
   #86's key_has_prime_order() was dead. Kept the live function, folded #86's
   fuller rationale onto it, deleted the duplicate.

3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of
   test_ed25519_identity_key_forgery_rejected, main() calling it twice and two
   tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery
   referencing k_low_order[] and messages[] that the merge had dropped.

   While restoring the corpus, corrected it (review finding on #86): the array
   claimed to hold "the eight low-order point encodings" and held five. Every
   order here was computed rather than copied — decode y, recover x, add the
   point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8,
   8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8
   encodings. D9FF..FF was in the array and is not a low-order point at all —
   no x satisfies the curve equation for that y — so it moves to a separate
   k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1).

   tests_run was assigned a literal (11) in main() and never incremented,
   which is how the duplicate call and the two unregistered tests went
   unnoticed. The TEST macro now increments it, so the total cannot drift.

Verified:
  cmake -DEBLDR_BUILD_TESTS=ON on master   FAILS to build, 3 errors
  same with this commit                    builds clean
  ctest                                    21/21 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   21/21 PASS
  pytest tests/                            24 passed, 1 skipped
  test_ed25519                             14/14 PASS (was 11 claimed, 12 run)
  discrimination, with `public_key_is_valid_subgroup` disabled:
    test_ed25519_low_order_keys_rejected   FAILS, as it must
    test_ed25519_non_canonical_...         still PASSES — those are refused by
      unpackneg() on canonicality, a different mechanism, which is the reason
      they are held in a separate array rather than counted among the eight.

* fix(fuzz): point every harness at the API it claims to fuzz

Four of the five fuzz harnesses did not test what they name, and the
build could not say so because not one of them includes a project
header. Each declares its target itself:

  fuzz_recovery_protocol.c  extern int eos_recovery_parse_packet(...)
  fuzz_fw_update.c          extern int eos_fw_update_init(void)
                            extern int eos_fw_update_process_chunk(...)
                            extern int eos_fw_update_finalize(void)
  fuzz_bootctl.c            extern int eos_bootctl_parse(...)
  fuzz_image_verify.c       extern int eos_image_parse_header(const void *, size_t)

eos_recovery_parse_packet, eos_fw_update_init,
eos_fw_update_process_chunk and eos_bootctl_parse have never existed —
those names appear nowhere outside the harness that declares them, so
three of the five targets have never linked.

The other two are worse, because they do link:

  - eos_fw_update_finalize exists but takes (ctx, mode), not (void).
  - eos_image_parse_header takes (uint32_t addr, eos_image_header_t *),
    not (const void *, size_t). fuzz_image_verify has been passing a
    pointer where an address is expected and a length where an output
    struct is expected, on every input, for as long as it has run.

A local extern is why none of this was caught: it tells the compiler the
symbol exists with whatever shape the harness asserts, and the mismatch
survives to link time or past it.

All five now include the real header and drive the real entry points:

  fuzz_image_verify       parse_header -> verify_integrity, boot-path order
  fuzz_bootctl            bootctl_load, then trap if load() accepted a
                          block validate() rejects
  fuzz_fw_update          begin -> write in fuzz-chosen chunk widths ->
                          finalize or abort
  fuzz_recovery_protocol  write_in_range, trapping on any accepted write
                          that leaves the slot or wraps, checked in wider
                          types that cannot
  fuzz_crypto             already correct; switched to the header so it
                          stays that way

Adds tests/fuzz/fuzz_sim_flash.h: the three harnesses that reach flash
need board ops installed, and without them they fuzz a null op table.

That the compiler now checks this is not theoretical — writing this, it
rejected EOS_UPGRADE_MODE_TEST for EOS_UPGRADE_TEST immediately, which
is exactly the error class the externs were hiding.

Verified. libFuzzer is unavailable here (libclang_rt.fuzzer_osx.a not
found), so each harness was linked against eboot_core and driven by a
standalone seed generator under ASan+UBSan:

  all five compile -Wall -Wextra    5/5, 0 failures
  20,000 inputs each               100,000 total, no reports
  ctest                            21/21 passed

Third instance of this class across these repos, after eos#50
(fuzz_devicetree naming eos_dtb_parse) and eos#110 (fuzz_ota_header
naming eos_ota_parse_header). The common factor every time is a fuzz
target declaring its own subject.

Stacked on #94, without which include/eos_image.h does not compile.

* fix(fuzz): the recovery oracle was off by one, and CI proved it

The restored Fuzz Harness Build job on #101 ran these harnesses under
real libFuzzer for the first time and fuzz_recovery_protocol trapped
within seconds. That was my bug, not the code's.

The oracle computed the one-past-the-end address:

    end = base + offset + len;  if (end > 0xFFFFFFFF) trap;

The last byte a write of len bytes touches is base + offset + len - 1.
eos_recovery_write_in_range() checks exactly that:

    if ((uint32_t)len - 1u > UINT32_MAX - (base + offset)) reject;

So an accepted write whose final byte lands exactly on 0xFFFFFFFF --
base=0xFFFFFF00, offset=0, len=256 -- is legal, the function accepts it,
and the old oracle trapped on it. A harness that traps on valid input
reports the opposite of the truth.

Verified both directions:
  the boundary case, fed directly      -> no trap
  a fail-open stub accepting all input -> trap fires (exit 133)
  500,000 random inputs, ASan+UBSan    -> no reports

libFuzzer doing precisely what it is for -- driving inputs into the one
corner a hand-written oracle got wrong -- is the strongest argument yet
for #101's job. The guard could never have caught this; only execution
under a coverage-driven fuzzer did.

* ci(fuzz): compile the harnesses, and refuse the extern that hid the bugs

#99 repairs the five harnesses in tests/fuzz/. Neither it nor anything
else stops the same thing happening again, because two of the conditions
that produced it are still in place.

**Nothing compiles them.** tests/fuzz/ is behind EBLDR_BUILD_FUZZ, which
defaults OFF, and no job set it. That is the whole reason three harnesses
could name functions that exist nowhere -- eos_bootctl_parse,
eos_recovery_parse_packet, eos_fw_update_init and friends -- and sit there
for as long as they did. A harness that is not built cannot fail to build.
The fuzz-build job restores that: clang, EBLDR_BUILD_FUZZ=ON (and
EBLDR_BUILD_TESTS=ON, since tests/fuzz is added from tests/CMakeLists.txt,
so the option alone would build nothing and the job would pass having
compiled no harness at all), then a five-second smoke run of each target.
It does not fuzz; a real campaign belongs in a scheduled workflow.

**The `extern` is still legal.** Every one of the four defects came from a
hand-written prototype standing in for an #include -- a promise the
compiler is obliged to believe and has no way to check. It is what made
the fourth defect invisible even to a compiler: fuzz_image_verify declared
eos_image_parse_header as (const void *, size_t) when it is
(uint32_t, eos_image_header_t *), so that harness linked, ran, and fuzzed
nothing. tests/unit/test_fuzz_harnesses.py refuses the pattern, and also
checks that every harness is built by a CMake target, links eboot_core,
and defines LLVMFuzzerTestOneInput.

The two halves cover different failures: the guard is static and stops the
pattern being written, the job is a compiler and catches a signature that
drifts under a harness that includes the right header. Neither subsumes
the other.

Mutation-checked: restoring one extern prototype fails
test_no_harness_declares_its_own_prototypes; deleting one add_executable
fails test_every_harness_is_built.

Verified: pytest tests/unit 26 passed 1 skipped, ctest 21/21. The fuzz job
itself could not be run here -- Apple clang ships no libclang_rt.fuzzer, so
-fsanitize=fuzzer will not link on this host. Instead each of #99's five
harnesses was compiled and linked against eboot_core with a stand-in driver
under -fsanitize=address,undefined and run over ~20k pseudo-random inputs
plus every length from 0 to 200; all five clean. That covers the symbol
resolution and the harness logic, which is what the job's build step
checks; it does not prove the libFuzzer link itself, which only CI can.
srpatcha pushed a commit that referenced this pull request Sep 8, 2026
* fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken

master (22d8f8b) does not compile. Two independent double-merges, both the
same shape: two PRs fixing adjacent things landed on stale bases, each was
green on its own branch, and the result was never rebuilt.

1. include/eos_image.h — #93 replaced reserved[30] with tlv_len (2) +
   tlv_hash[28], preserving every offset. #87 merged afterwards carrying
   asserts written against the older struct:

     error: no member named 'reserved' in 'eos_image_header_t'   (x2)

   #93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert
   was a duplicate; the width assert had no replacement and is restored as two
   asserts covering both halves of the same 30-byte span. No offset moves and
   the wire format is unchanged.

2. core/ed25519_verify.c — #86 and #57 both landed a subgroup guard, so the
   file carried two byte-identical point_is_identity() definitions:

     error: redefinition of 'point_is_identity'

   Only #57's public_key_is_valid_subgroup() is wired to the call site, so
   #86's key_has_prime_order() was dead. Kept the live function, folded #86's
   fuller rationale onto it, deleted the duplicate.

3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of
   test_ed25519_identity_key_forgery_rejected, main() calling it twice and two
   tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery
   referencing k_low_order[] and messages[] that the merge had dropped.

   While restoring the corpus, corrected it (review finding on #86): the array
   claimed to hold "the eight low-order point encodings" and held five. Every
   order here was computed rather than copied — decode y, recover x, add the
   point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8,
   8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8
   encodings. D9FF..FF was in the array and is not a low-order point at all —
   no x satisfies the curve equation for that y — so it moves to a separate
   k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1).

   tests_run was assigned a literal (11) in main() and never incremented,
   which is how the duplicate call and the two unregistered tests went
   unnoticed. The TEST macro now increments it, so the total cannot drift.

Verified:
  cmake -DEBLDR_BUILD_TESTS=ON on master   FAILS to build, 3 errors
  same with this commit                    builds clean
  ctest                                    21/21 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   21/21 PASS
  pytest tests/                            24 passed, 1 skipped
  test_ed25519                             14/14 PASS (was 11 claimed, 12 run)
  discrimination, with `public_key_is_valid_subgroup` disabled:
    test_ed25519_low_order_keys_rejected   FAILS, as it must
    test_ed25519_non_canonical_...         still PASSES — those are refused by
      unpackneg() on canonicality, a different mechanism, which is the reason
      they are held in a separate array rather than counted among the eight.

* ci: gate what the repository actually reports, and close the sibling gate that failed open

Answers the review on #90.

Finding 1 (Medium) -- `CI Gate` gates three jobs in ci.yml. This repository has
16 workflow files and a PR head reports 26 checks across five runs, so the
maintainer action in the body -- require one name -- would have left CodeQL,
every EoSim platform leg, all three Cross-Platform legs and a second host build
unrequired, and `master` could still go red from any of them. The rot-guard had
the same boundary: WORKFLOW was hardcoded to ci.yml, so a job added to
build.yml or codeql.yml was neither covered nor noticed.

Adds two tests over every workflow with a `pull_request` trigger:
REQUIRED_CHECKS names what a maintainer must actually require, NO_GATE excuses
the rest with a reason about the workflow itself, and a third test asserts each
gated workflow really has a job displaying under the name given.

Finding 2 (Medium) -- `Simulation Gate` had the exact fail-open shape this PR
removes from ci.yml: `needs: [simulate, cross-platform]`, then it printed both
results and branched on `simulate` alone before printing "All simulation checks
passed". A red or skipped `cross-platform` -- three OS legs -- passed it. Ported
the `toJSON(needs)` + `jq` body, which is dependency-list-agnostic and cannot
fall out of step with `needs:`.

And made it a rule rather than a one-off:
test_no_aggregating_gate_ignores_part_of_its_needs walks every gate in every
pull-request workflow and fails if a declared dependency is never compared.
Two refinements were needed to make it mean something:

  - printing a result is not testing it. The first version grepped for
    `needs.X.result` anywhere in the script, which Simulation Gate satisfied
    with its echo line. Only a line that compares counts.
  - book-build.yml's `summary` writes a step summary and claims no verdict.
    It is reporting, not gating, so the check applies only to jobs that either
    `exit 1` or assert that everything passed.

Finding 3 (Low) -- this branch had replaced master's
`pip3 install -r requirements.txt pytest-cov` with a hand-maintained list to
add pyyaml, which regressed the guard in tests/unit/test_requirements.py:

    these jobs run pytest over tests/ but never install from requirements.txt
    ... ['ci.yml:test']

Restored the requirements.txt install; pyyaml was already declared there, so
the hand list was not needed at all. That also removes the collision with #88,
which edits the same line.

Verified:
  pytest tests/                          47 passed
  ctest                                  21/21 PASS
  yaml.safe_load of ci.yml, simulation-test.yml, book-build.yml   all parse
  discrimination, both ways:
    - reverting Simulation Gate to its fail-open body gives
        simulation-test.yml:sanity-gate declares needs
        ['simulate', 'cross-platform'] but never tests ['cross-platform']
    - with the toJSON(needs) body in place, 9 passed

Refs #90

* ci: execute the gate rule in a test, instead of grepping for it

Finishes finding 3 from the review on #90. Findings 1 and 2 were already
addressed in 1bd6a22; this is the one left.

test_gate_fails_on_any_non_success_result searched the gate's `run:` text for
`!= "success"` and `exit 1`. That is a string match on an implementation, not a
check of behaviour: it passes for those tokens sitting in a comment or an
unreachable branch, and fails for a correct rewrite expressing the same rule
differently. The only real evidence that the rule works was the fork
experiment, which is a one-off nothing re-runs.

The rule now lives in .github/scripts/ci-gate-check.sh, and pytest runs it
against real inputs: success, failure, skipped, cancelled, a mixed set, and an
empty context. Both gates -- ci.yml's `CI Gate` and simulation-test.yml's
`Simulation Gate` -- call that one script, and a further test asserts they do,
so the behavioural cases cover every gate rather than one.

The script also refuses an empty or null `needs` context. No results is not the
same as no failures, and a gate that passes when it was handed nothing is the
same fail-open shape in a different place.

Verified by mutation: weakening the rule to `== "failure"` fails 3 of 18, and a
gate that stops calling the script fails 1 of 18. Restored, 18 pass.

42 Python tests pass (1 skipped), ctest 20/20, all 16 workflow files parse.
srpatcha pushed a commit that referenced this pull request Sep 8, 2026
* fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken

master (22d8f8b) does not compile. Two independent double-merges, both the
same shape: two PRs fixing adjacent things landed on stale bases, each was
green on its own branch, and the result was never rebuilt.

1. include/eos_image.h — #93 replaced reserved[30] with tlv_len (2) +
   tlv_hash[28], preserving every offset. #87 merged afterwards carrying
   asserts written against the older struct:

     error: no member named 'reserved' in 'eos_image_header_t'   (x2)

   #93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert
   was a duplicate; the width assert had no replacement and is restored as two
   asserts covering both halves of the same 30-byte span. No offset moves and
   the wire format is unchanged.

2. core/ed25519_verify.c — #86 and #57 both landed a subgroup guard, so the
   file carried two byte-identical point_is_identity() definitions:

     error: redefinition of 'point_is_identity'

   Only #57's public_key_is_valid_subgroup() is wired to the call site, so
   #86's key_has_prime_order() was dead. Kept the live function, folded #86's
   fuller rationale onto it, deleted the duplicate.

3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of
   test_ed25519_identity_key_forgery_rejected, main() calling it twice and two
   tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery
   referencing k_low_order[] and messages[] that the merge had dropped.

   While restoring the corpus, corrected it (review finding on #86): the array
   claimed to hold "the eight low-order point encodings" and held five. Every
   order here was computed rather than copied — decode y, recover x, add the
   point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8,
   8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8
   encodings. D9FF..FF was in the array and is not a low-order point at all —
   no x satisfies the curve equation for that y — so it moves to a separate
   k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1).

   tests_run was assigned a literal (11) in main() and never incremented,
   which is how the duplicate call and the two unregistered tests went
   unnoticed. The TEST macro now increments it, so the total cannot drift.

Verified:
  cmake -DEBLDR_BUILD_TESTS=ON on master   FAILS to build, 3 errors
  same with this commit                    builds clean
  ctest                                    21/21 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   21/21 PASS
  pytest tests/                            24 passed, 1 skipped
  test_ed25519                             14/14 PASS (was 11 claimed, 12 run)
  discrimination, with `public_key_is_valid_subgroup` disabled:
    test_ed25519_low_order_keys_rejected   FAILS, as it must
    test_ed25519_non_canonical_...         still PASSES — those are refused by
      unpackneg() on canonicality, a different mechanism, which is the reason
      they are held in a separate array rather than counted among the eight.

* test(ed25519): hold both repos' verifiers to one shared contract

eos and eBoot each carry their own Ed25519 verifier. Different code,
opposite return conventions — eos returns 1 to accept, eBoot returns
EOS_OK — doing the same job on the same wire format. For a while only
eos rejected low-order public keys, and nothing in either repo could
notice: there is no shared build, and the implementations are far too
different for a source diff to say anything.

That divergence is why #73 survived in eBoot after eos had already fixed
it. Fixing eBoot closes the hole; it does not stop the next crypto fix
in either repo from failing to reach the other.

What can be shared is the data. tests/vectors/ed25519_contract_vectors.h
is 76 vectors of pure test data with a SHA-256 over them:

  64  low-order keys — the eight points of order dividing 8, each over
      eight messages. A key of order n makes (R = identity, S = 0)
      verify whenever n divides SHA-512(R||A||M), about one message in
      n, so a test pinned to a single message passes against unfixed
      code.
   3  RFC 8032 section 7.1 signatures that must verify
   6  the same three with one bit flipped in R or in S
   3  the same three with S + L, non-canonical per RFC 8032 5.1.7

The intended twin is tests/test_ed25519_contract.c in eos, compiling the
byte-identical header. Each side runs its own verifier and prints the
digest; a change to one copy that does not reach the other shows up as
two different digests, which is the part a diff could never give.

The header is generated, so the corpus is reproducible rather than
transcribed:  python3 tools/gen_ed25519_contract_vectors.py

The three positive vectors are load-bearing and the test says so: a
verifier that refuses everything satisfies all 73 negative vectors, so
it also fails if the corpus ever loses its accept cases.

Verified:
  on this branch (with the #86 fix) -> 76/76, 3 accepted, 73 refused
  against unfixed origin/master     -> 28 of 76 wrong, exit 1
  ctest                             -> 22/22 passed
  digest 1059febedae2b3e3dfaa1ef5b419fb37ed7f0d53b377a3250b53b95a546282a2

* build(tests): generate the Ed25519 contract vectors, do not commit them

tests/vectors/ed25519_contract_vectors.h is 54 KB of generated data, and the
identical file was checked in to BOTH eos and eBoot, along with the generator
that produces it -- 61 KB duplicated verbatim across two repositories and kept
in step by hand.

The generator is pure stdlib and deterministic, so each repo can produce the
header into its own build tree instead. tools/gen_ed25519_contract_vectors.py
grows an -o/--output flag (stdout stays the default, so the regeneration
command in its docstring still works), and tests/CMakeLists.txt drives it from
add_custom_command. Only the 6.8 KB generator is now carried twice, and it is
source rather than generated output.

Verified: the generated header is byte-identical to the file it replaces; both
repos still report digest
1059febedae2b3e3dfaa1ef5b419fb37ed7f0d53b377a3250b53b95a546282a2, which is the
thing that actually holds the two verifiers to one contract. Suites pass (eos
35/35, eBoot 22/22). Breaking the generator fails the build rather than
silently reusing a stale header. The cross-compile legs configure with tests
OFF so find_package(Python3) is never reached there; confirmed by configuring
under a cross toolchain.

The test's include of the vector header becomes "vectors/..." rather than
"../vectors/...": a path relative to the source file cannot reach the
generated copy in the build tree.

* test(ed25519): assert the contract digest instead of printing it

Answers the review on #89.

Finding 1 (High) -- the digest was printf'd and never compared to anything,
so the drift guard the PR is named for did not exist. Worse, it could not:
EOS_ED25519_CONTRACT_DIGEST came from the header this repo's own generator had
just written, a hash taken over its own output. Editing the generator on one
side produced a self-consistent corpus with a new digest, a green test, and a
divergence visible only to a human reading two CI logs in two repositories.
Three places asserted the guarantee -- the PR body, this file's docblock, and
the generator's emitted header comment -- and none implemented it. The unused
<string.h> include was the strcmp that never got written.

Now pinned as a literal in committed source, with the vector count and the
accept count beside it, because those are generated too: a corpus that shrank
from 76 to 40, or lost two of its three RFC 8032 positives, previously passed.
positives == 0 only fired when the last positive went.

Finding 2 (Medium) -- tests/vectors/ed25519_contract_vectors.h is now
committed rather than generated into the build tree. What the TCB's signature
verifier was tested against should be visible from a tag. The reason it was
generated -- two repos holding the same file and keeping it in step by hand --
is what the pinned digest now handles. That also drops
find_package(Python3 ... REQUIRED), which had made a Python interpreter a hard
configure-time dependency of a pure-C test suite.

Finding 3 (Medium) -- #86 has merged, so this branch is rebased onto master
and reduced to its own three files. It previously carried all of #84 and #85
(device tree parsing, +475 lines, unrelated to Ed25519) plus #86's verifier
change, none of which was declared in the body.

Verified:
  ctest                                     22/22 PASS
  test_ed25519_contract                     76 vectors, 3 accept, 73 refuse
  the guard now discriminates: adding one vector to the generator gives
      [FAIL] corpus digest changed
             expected 1059febe...282a2
             got      e08b0632...28905
    and exit 1. Before this commit the same edit printed a different digest
    and exited 0.
  cmake configure with no Python on PATH     succeeds (no find_package)

Refs #89

* test(ed25519): hash the vectors, not a sibling #define

Answers the second review on #89. Finding 1 is a defect in my own previous
commit, and it is the same class that commit was written to fix.

Finding 1 (High) -- the pin did not cover the corpus it ships with.
EOS_ED25519_CONTRACT_DIGEST is a #define inside the generated header, written
there by the generator; EOS_ED25519_CONTRACT_EXPECTED is a #define in this
file. The test compared one string literal to another. Nothing hashed the
vector bytes, so editing a committed vector changed the data and left the
value it was compared against untouched.

The reviewer demonstrated it by changing one byte of the order-1 identity
public key -- the vector that pins the Critical secure-boot bypass -- and the
suite still reported "all 76 contract vectors behaved as specified". The guard
fired only when someone re-ran the generator: the file it protects was the one
file it could not see. Committing the corpus in the previous commit is exactly
what made that gap matter.

contract_digest() now recomputes SHA-256 over the vectors at run time, in the
generator's serialisation -- public_key[32] || signature[64] ||
message[message_len] || (uint8_t)expect_accept, per vector, in order -- using
eos_sha256_* from eos_crypto_boot.h, which this file already included. Two
comparisons follow: the computed digest against the pinned literal, and the
computed digest against the header's own claim, so a corpus and a header from
different generator runs are caught too.

  Reproduced the reviewer's probe. With one byte of the identity key changed:
      before this commit:  [PASS] all 76 contract vectors behaved as specified
      after:               [FAIL] corpus digest changed
                                  expected 1059febe...282a2
                                  got      55370deb...cae9
                           exit 1

Finding 2 (Low) -- three places in committed source asserted the guarantee
finding 1 showed was not implemented, the generator docstring most explicitly:
"if either copy is edited, that side's build fails and the digests visibly
stop matching". It did not. With finding 1 fixed the claim is true, and all
three are reworded to say what now actually holds -- that the digest is
recomputed over the bytes, so a hand-edited vector fails as loudly as a
regenerated corpus. That distinction is the point: the hand edit is the one a
reviewer would not catch in a diff of 76 byte arrays.

The regenerated header differs from the committed one only in that comment;
the digest and every vector are byte-identical.

Verified:
  ctest                              22/22 PASS
  pytest tests/                      38 passed
  test_ed25519_contract              76 vectors, 3 accept, 73 refuse
  byte-flip probe                    FAILS, as shown above
  regenerate + diff                  data unchanged, comment only

Refs #89
srpatcha added a commit that referenced this pull request Sep 8, 2026
…85)

* fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken

master (22d8f8b) does not compile. Two independent double-merges, both the
same shape: two PRs fixing adjacent things landed on stale bases, each was
green on its own branch, and the result was never rebuilt.

1. include/eos_image.h — #93 replaced reserved[30] with tlv_len (2) +
   tlv_hash[28], preserving every offset. #87 merged afterwards carrying
   asserts written against the older struct:

     error: no member named 'reserved' in 'eos_image_header_t'   (x2)

   #93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert
   was a duplicate; the width assert had no replacement and is restored as two
   asserts covering both halves of the same 30-byte span. No offset moves and
   the wire format is unchanged.

2. core/ed25519_verify.c — #86 and #57 both landed a subgroup guard, so the
   file carried two byte-identical point_is_identity() definitions:

     error: redefinition of 'point_is_identity'

   Only #57's public_key_is_valid_subgroup() is wired to the call site, so
   #86's key_has_prime_order() was dead. Kept the live function, folded #86's
   fuller rationale onto it, deleted the duplicate.

3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of
   test_ed25519_identity_key_forgery_rejected, main() calling it twice and two
   tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery
   referencing k_low_order[] and messages[] that the merge had dropped.

   While restoring the corpus, corrected it (review finding on #86): the array
   claimed to hold "the eight low-order point encodings" and held five. Every
   order here was computed rather than copied — decode y, recover x, add the
   point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8,
   8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8
   encodings. D9FF..FF was in the array and is not a low-order point at all —
   no x satisfies the curve equation for that y — so it moves to a separate
   k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1).

   tests_run was assigned a literal (11) in main() and never incremented,
   which is how the duplicate call and the two unregistered tests went
   unnoticed. The TEST macro now increments it, so the total cannot drift.

Verified:
  cmake -DEBLDR_BUILD_TESTS=ON on master   FAILS to build, 3 errors
  same with this commit                    builds clean
  ctest                                    21/21 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   21/21 PASS
  pytest tests/                            24 passed, 1 skipped
  test_ed25519                             14/14 PASS (was 11 claimed, 12 run)
  discrimination, with `public_key_is_valid_subgroup` disabled:
    test_ed25519_low_order_keys_rejected   FAILS, as it must
    test_ed25519_non_canonical_...         still PASSES — those are refused by
      unpackneg() on canonicality, a different mechanism, which is the reason
      they are held in a separate array rather than counted among the eight.

* fix(fdt): bound a device tree parser that was never compiled

core/fdt_loader.c is in no source list, so it has never been built. The
header offers it to callers, rtos_boot.c is named as the consumer, and
nothing catches what is in it -- not the compiler, not ctest, not the
sanitizer job.

What is in it is a parser for a blob that comes out of flash, whose every
header field it uses as an offset or a length without checking any of
them. eos_fdt_validate() looks at magic and version only, so it accepts
a blob whose off_dt_struct points anywhere:

    hdr.totalsize     = 40      (the header alone)
    hdr.off_dt_struct = 0x100000
    eos_fdt_validate(blob) -> 0
    eos_fdt_get_prop(...)  -> AddressSanitizer: BUS, READ at
                              fdt_loader.c:25 in fdt_read_u32

Five more, all reachable the same way:

* the tag read at the top of the loop takes 4 bytes where the loop
  condition guarantees 1;
* strlen() on a node name reads until it finds a zero, which for a name
  running to the end of the block is past it;
* nameoff indexes the strings block unchecked, so strcmp() reads from an
  arbitrary address;
* a property len is clamped to the caller's buffer before the memcpy,
  which bounds the write but not the read -- an oversized len copies
  whatever follows the blob out to the caller;
* FDT_END_NODE decrements depth with no floor.

Bound them. validate() is the gate every path goes through, so the block
offsets are checked against totalsize there, and get_prop() calls it
before trusting the header. Inside the loop each read is checked for the
width it takes, the name and property-name scans are bounded by memchr
within their blocks, and the padded advances are re-checked for overrun.

Adds the file to eboot_core and tests/unit/test_fdt_loader.c to ctest:
ten cases, one well-formed tree that must still parse and nine malformed
ones. Against the unfixed parser the suite fails on the third; with the
bounds in place the whole suite is 20/20, and 20/20 under EBLDR_SANITIZE.

Not fixed here, because it is a behaviour change rather than a safety
one: node paths below the root do not resolve. _get_prop derives the
depth to match from a slash count, so "/chosen" looks for depth 1, which
is the root -- "chosen" is at depth 2. Only "/" resolves today.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(fdt): bound the parser against the caller's length, not the blob's claim

Follow-up to the review on #84. Three findings from it, all in files this
branch already owns.

Finding 1 (High) -- every bound in eos_fdt_validate() was expressed relative
to hdr->totalsize, which is read out of the same untrusted blob. Checking the
block offsets against it proves the header is internally consistent and says
nothing about how many bytes are really mapped: a 40-byte buffer declaring
totalsize = 0x100000 with agreeing offsets passed every check, and
eos_fdt_get_prop() then walked a megabyte past the allocation. The original
reproducer (an inflated *offset* against an honest totalsize) was rejected;
its mirror image was not.

Adds the length-carrying entry points eos_fdt_validate_sized() and
eos_fdt_get_prop_sized(), which take the bytes the caller actually owns and
check totalsize against that before anything else. The existing two-argument
forms stay as wrappers that trust the blob's own totalsize, documented in the
header as a warrant the caller has to make good; eos_fdt_load() now passes
max_size, which it had all along.

Finding 2 (Medium) -- a property larger than the caller's buffer was copied
short and reported as success, so a clipped value was indistinguishable from
a complete one. In a boot path this reads bootargs. Now returns -7 and sets
*buf_len to the full length so the caller can size a retry.

Finding 3 (Medium) -- adds tests/fuzz/fuzz_fdt.c alongside the five existing
harnesses. .ai/security.md names device tree among the parsers that get fuzz
coverage rather than unit tests alone. It drives the sized entry points and
passes the real size, which is what makes the harness meaningful -- the
unsized forms would let a fuzzer authorise its own out-of-bounds read.

Finding 6 (Low) -- drops the unreachable `total < sizeof(fdt_header_t)` check
in eos_fdt_load(); validate() already rejects that blob, and the comment
above it described a bound it was not applying.

Header now documents the full -1..-8 return code table.

Verified:
  cmake -DEBLDR_BUILD_TESTS=ON, ctest              21/21 PASS
  same under -DEBLDR_SANITIZE=ON (ASan+UBSan)      21/21 PASS
  test_fdt_loader                                  14 tests PASS (was 10)
  new tests against the pre-fix parser             both FAIL as they should
    - inflated totalsize accepted at the sized entry point
    - truncating call returned 0 instead of -7
  grep for eos_fdt_get_prop / eos_fdt_validate     no callers outside the
    parser and its tests, so the -7 change breaks nothing today

Refs #84

* fix(build): drop the duplicate core/fdt_loader.c registration

master gained core/fdt_loader.c in eboot_core while this branch was open, so
after the rebase it appeared twice and
tests/unit/test_cmake_core_sources.py::test_core_sources_are_registered_once
failed:

    AssertionError: ['core/fdt_loader.c'] is not false :
    duplicate eboot_core sources: ['core/fdt_loader.c']

Kept master's entry, dropped the one this branch added. That guard is the
same shape as the one this stack adds for toolchain specs -- a build-file
check that catches the class rather than the instance -- and it did its job.

Verified: ctest 22/22 PASS, pytest 24 passed 1 skipped.

Refs #84

* fix(fdt): one length-carrying API, and a CI job that builds the fuzz harnesses

Answers the second review on #84.

Finding 2 (Medium) -- eos_fdt_validate() and eos_fdt_get_prop() kept an
out-of-bounds read by design. Both dereferenced hdr->totalsize before any
length was known and handed that attacker-controlled value to the _sized form
as its bound, so calling either on a short buffer read past it before a single
check had run. The header called that a caller warrant; a warrant is not a
check, and it is the exact bug this PR exists to fix. Neither had an in-tree
caller.

Removed rather than documented. There is now one form of each entry point and
it always takes the length:

    int eos_fdt_validate(const void *fdt_blob, uint32_t avail);
    int eos_fdt_get_prop(const void *fdt, uint32_t fdt_len, ...);

An exported unsafe twin in a TCB header is a future boot-path caller
reintroducing the bug with no compiler complaint, which is worth more than the
convenience of a one-argument call. eos_fdt_load() already passed max_size.
The test that pinned the wrapper's behaviour is gone with the wrapper, and
get_prop_sized_exact() collapsed into get_prop_exact() since they became the
same function.

Finding 3 (Medium) -- tests/fuzz/ was built by nothing. EBLDR_BUILD_FUZZ
defaults OFF and no job set it, so the harness added here joined five others
that no CI job compiles. A harness that is never built cannot fail to build,
which is how fuzz_devicetree came to declare a function that did not exist and
sit there unnoticed (eos#50).

Adds a `fuzz-build` job: configure with clang, build every harness, and run
each for five seconds over its own generated inputs. That is not a campaign --
it is enough to catch a harness that no longer compiles or crashes at once,
which is the failure this repo has actually had. Note it needs
EBLDR_BUILD_TESTS=ON as well: tests/fuzz/ is added from tests/CMakeLists.txt,
so EBLDR_BUILD_FUZZ alone configures cleanly and builds no harness at all --
the job would have passed having compiled nothing. Found that locally before
writing the job, not after.

  NOT RUN, and this is the honest limit: this host has no libFuzzer runtime
  (libclang_rt.fuzzer_osx.a is absent from the Xcode toolchain), so the link
  step cannot be reproduced here for any harness, old or new. What I verified
  is that CMake configures with both flags and reports all six targets --
  "Fuzz targets: fuzz_image_verify, fuzz_recovery_protocol, fuzz_fw_update,
  fuzz_crypto, fuzz_bootctl, fuzz_fdt" -- and that fuzz_fdt.c passes
  `cc -fsyntax-only`. The link and the smoke run are CI's to show, and this
  job is what makes them visible.

  Coordination note: #90 adds a `CI Gate` whose needs list is
  [test, build-arm, static-analysis]. Whichever of #84 and #90 lands second
  must add fuzz-build to that list -- and #90's own
  test_gate_covers_every_job_that_runs_on_a_pull_request fails loudly if it is
  not, which is the guard working rather than a trap.

Finding 1 (High) is a PR-body correction, made there: the diff carries #94's
master repair because this branch is stacked on it, and the body described
only the FDT parser.

Verified:
  ctest                                    22/22 PASS
  pytest tests/                            38 passed
  test_fdt_loader                          13 tests PASS
  grep for a length-free entry point       none remains in the header

Refs #84

* ci: move the fuzz-harness build to its own PR

The fuzz-build job added in the previous commit does its job -- it is red, on
harnesses this PR did not write:

    undefined reference to `eos_fw_update_init'
    undefined reference to `eos_fw_update_process_chunk'
    undefined reference to `eos_recovery_parse_packet'

None of those exist anywhere in the tree. tests/fuzz/fuzz_fw_update.c and
fuzz_recovery_protocol.c declare an API that was never written, exactly as
eos#50's fuzz_devicetree declared eos_dtb_parse(). They have compiled forever
because nothing ever linked them.

That is a real finding and it deserves a fix, not a red tick on an unrelated
PR. The job and the repairs move to their own change; this PR keeps
tests/fuzz/fuzz_fdt.c, which is the harness it is responsible for.

Refs #84

* fix(fdt): read the header the way the struct block is read, and stop resynchronising on garbage

Answers the third review on #84.

Finding 3 (Low, and the one that matters on hardware) -- the header was read
through a cast fdt_header_t* and direct member loads, while every struct-block
read goes through the fdt_read_u32() memcpy helper precisely because the blob
may be unaligned. Fuzz input, a buffer inside a larger message, a copy at an
odd offset: nothing promises 4-byte alignment, and on a strict-alignment
cross target a direct member load is the same fault class this parser exists
to avoid. The unit suite could never catch it -- blob_t.bytes happens to be
aligned.

All header fields now go through fdt_hdr_u32() (memcpy at offsetof), one rule
for the whole blob, in validate(), load() and get_prop(). FUZZ_FLAGS gains
`undefined` so the fuzz job checks alignment too.

Finding 4 (Low) -- `default: break;` resynchronised on unrecognised tags: the
walk skipped 4 bytes and treated whatever followed as the next token, so a
struct block of arbitrary bytes parsed to a clean "not found". Bounded, but a
TCB parser that walks garbage to completion is accepting input it does not
understand. FDT_NOP -- the one legal unknown, padding the spec allows between
tokens -- is now named in the header and passes; anything else returns -6.

Findings 1, 2 and the -8 line: the fuzz-build job's single home is #101
(nothing added here; the harness is inert until that job lands and then
compiled by it -- said on the thread, not just here), and the header no longer
documents return code -8, which nothing at this head returns. #85 adds the -8
return and re-documents it together with the FDT_MAX_PATH_DEPTH move.

Verified:
  ctest                                    22/22 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   22/22 PASS
  pytest tests/                            38 passed
  test_fdt_loader                          16 tests PASS (was 13)
  discrimination, each fix in isolation:
    default: break restored     -> test_a_garbage_tag_is_refused_not_skipped
                                   FAILS (expects -6, gets "not found")
    direct member load restored -> under UBSan the new unaligned-blob test
                                   reports "load of misaligned address
                                   0x...671 for type 'const uint32_t'" at the
                                   exact line -- and cannot fire on the fixed
                                   code, which runs the same test clean
  NOP counter-check: interleaved FDT_NOP tokens still resolve, so the
  stricter default does not reject real trees.

Refs #84

* fix(fdt): resolve a node by its path, not by its depth and last name

Replaces slash-counting and last-component matching with a component walk
gated on ancestor matches, so /soc/uart and /decoy/uart are distinguishable.

Also carries the fixes from the review on this PR:

Finding 1 (High) -- the property arm tested in_target alone, and in_target is
cleared only by the target's own FDT_END_NODE, so it stayed true for the
entire subtree and a property found on a *child* was returned as the target's
own. On this PR's own decoy tree, eos_fdt_get_prop(blob, "/soc", "reg", ...)
returned 0 with "soc-uart" when /soc has no reg property at all and the
correct answer is -5. Real device trees emit properties before subnodes, so a
property that is present on the target is still found first -- the bug bites
when the target lacks it, turning "not found" into a silently wrong value
from a nested node. For an attacker-supplied blob the ordering is not
enforced at all. Fixed by also requiring depth == target_depth, which is
exact: depth is target_depth for the target's own properties and
target_depth + 1 or deeper inside any child.

Finding 2 (Low) -- test_the_parent_of_the_node_has_to_match_too asserted a
second positive resolution rather than a rejection, so the suite read as
though it had a negative case for wrong parents when the negative coverage
actually lives in test_a_node_below_the_root_resolves. Renamed to
test_each_uart_returns_its_own_value, and the load-bearing property (decoy is
emitted before soc, so a last-component matcher would return "decoy-uart")
is now stated on the test that depends on it.

Finding 3 (Low) -- a path deeper than FDT_MAX_PATH_DEPTH returned -1, the
same code as a NULL argument, so a caller could not tell a programming error
from malformed input. Now -8, documented in the header's code table.

Verified:
  ctest                                            21/21 PASS
  same under -DEBLDR_SANITIZE=ON (ASan+UBSan)      21/21 PASS
  test_fdt_loader                                  20 tests PASS (was 14)
  test_a_property_on_a_child_is_not_returned_as_the_parents, run against
    this same parser with only the depth guard reverted:
      [FAIL] get_prop_exact(&b, "/soc", "reg", out, &len) != 0
    so it discriminates rather than restating the fix.

Refs #85

* fix(fdt): match node names past the unit address, where real device trees put it

Replaces slash-counting and last-component matching with a component walk
gated on ancestor matches, and matches a component against the node name up
to its unit address -- `/soc/uart` resolves `uart@40011000`, an explicit
`@40011000` still selects exactly that node, and an address not in the tree
does not fall back to a loose match.

Carries the earlier review fixes (depth == target_depth on the property arm;
FDT_MAX_PATH_DEPTH moved to the public header; -8 for an over-deep path,
distinct from -1) and, from the overnight round:

  - the stray `/* Deepest node path ... */` comment left in the .c after the
    define moved to the header is gone (Low)
  - return code -8 is documented in the header's code table again, on this
    branch, because this is the branch where anything returns it -- #84
    dropped the line for exactly that reason

Rebased onto #84's alignment/NOP round; the test-file merge keeps both sides'
suites (garbage-tag, NOP-padding and unaligned-blob from #84; the node-path
and unit-address suites from here).

Verified:
  ctest                                    22/22 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   22/22 PASS
  pytest tests/                            38 passed
  test_fdt_loader                          24 tests PASS

Refs #85

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Srikanth Patchava <srikanth.patchava@outlook.com>
srpatcha pushed a commit that referenced this pull request Sep 8, 2026
* fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken

master (22d8f8b) does not compile. Two independent double-merges, both the
same shape: two PRs fixing adjacent things landed on stale bases, each was
green on its own branch, and the result was never rebuilt.

1. include/eos_image.h — #93 replaced reserved[30] with tlv_len (2) +
   tlv_hash[28], preserving every offset. #87 merged afterwards carrying
   asserts written against the older struct:

     error: no member named 'reserved' in 'eos_image_header_t'   (x2)

   #93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert
   was a duplicate; the width assert had no replacement and is restored as two
   asserts covering both halves of the same 30-byte span. No offset moves and
   the wire format is unchanged.

2. core/ed25519_verify.c — #86 and #57 both landed a subgroup guard, so the
   file carried two byte-identical point_is_identity() definitions:

     error: redefinition of 'point_is_identity'

   Only #57's public_key_is_valid_subgroup() is wired to the call site, so
   #86's key_has_prime_order() was dead. Kept the live function, folded #86's
   fuller rationale onto it, deleted the duplicate.

3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of
   test_ed25519_identity_key_forgery_rejected, main() calling it twice and two
   tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery
   referencing k_low_order[] and messages[] that the merge had dropped.

   While restoring the corpus, corrected it (review finding on #86): the array
   claimed to hold "the eight low-order point encodings" and held five. Every
   order here was computed rather than copied — decode y, recover x, add the
   point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8,
   8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8
   encodings. D9FF..FF was in the array and is not a low-order point at all —
   no x satisfies the curve equation for that y — so it moves to a separate
   k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1).

   tests_run was assigned a literal (11) in main() and never incremented,
   which is how the duplicate call and the two unregistered tests went
   unnoticed. The TEST macro now increments it, so the total cannot drift.

Verified:
  cmake -DEBLDR_BUILD_TESTS=ON on master   FAILS to build, 3 errors
  same with this commit                    builds clean
  ctest                                    21/21 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   21/21 PASS
  pytest tests/                            24 passed, 1 skipped
  test_ed25519                             14/14 PASS (was 11 claimed, 12 run)
  discrimination, with `public_key_is_valid_subgroup` disabled:
    test_ed25519_low_order_keys_rejected   FAILS, as it must
    test_ed25519_non_canonical_...         still PASSES — those are refused by
      unpackneg() on canonicality, a different mechanism, which is the reason
      they are held in a separate array rather than counted among the eight.

* fix(eos_sign): emit the layout master defines, not a new meaning for hdr_size

Answers the review on #88. Both High findings had the same root, and master
has since settled the question the reviewer said needed a decision.

The original defect is real and unchanged: the tool emitted
[header][TLV][payload] while stamping hdr_size as a fixed 156, so
core/image_verify.c's `payload_addr = addr + hdr->hdr_size` landed on the TLV
block and every image it produced failed integrity verification on-device.

This branch fixed it by moving hdr_size to mean "offset to the payload". That
was the wrong half to move.

Finding 1 (High) -- eFirmware/src/efw_image.c:122 checks
`hdr_size != EFW_IMAGE_HDR_SIZE` for exact equality against 156, so every TLV
image would have been rejected by the other half of the format, which
eFirmware's header documents as interchangeable.

Finding 2 (High) -- core/rollback.c computes the TLV address as
image_addr + hdr_size + image_size, i.e. it assumes the area follows the
payload. Under either the old layout or this branch's, the anti-rollback
EOS_TLV_MIN_SEC_VER could never be found, so the counter this repository gates
downgrades on was unreachable for every image the tool produced.

Master answers both, and it is the reviewer's option (b): #93 landed tlv_len
and tlv_hash in the header -- inside EOS_IMG_SIGNED_LEN -- and documents that
"the TLV area sits after the payload". So this PR now emits
[header][payload][TLV] with hdr_size always 156. eFirmware's equality check
keeps working untouched, rollback.c's arithmetic is correct as written, and no
field changes meaning.

That reordering also resolves an ordering problem the old shape had: the TLV
carried the signature, so its digest could not be computed before the
signature existed. The signature now lives in the header's signature[] field
and the TLV carries metadata only, which is what makes tlv_hash signable.

Finding 4 (Medium) -- the two `assert`s in cmd_sign are removed by `python -O`.
This is release-signing tooling, so the signature-length check is now a
`raise SystemExit`. The TLV-length assert is gone with the code that needed it.

Finding 5 (Medium) -- `pytest.importorskip("cryptography")` still skipped
silently if the install ever failed, leaving "collected 19, ran 0" as a green
run. Now gated: EOS_REQUIRE_SIGNING_TESTS is set in ci.yml, and under it the
module is imported directly so a missing dependency is a collection error.
Locally, without the variable, the skip still applies.

Finding 6 (Low) -- cmd_verify bounds-checks the image length before unpacking,
so a short file gets a FAIL: line rather than a struct.error traceback, and
the 156 is hoisted to EOS_IMG_STRUCT_SIZE rather than re-derived.

Finding 3 (Medium) needs no change here: master's include/eos_image.h already
documents the layout this now emits.

Rebased onto #94 (the master repair) and reduced to the five files that are
still this PR's own -- the branch was stale enough that its diff against
master would have reverted core/rollback.c, include/eos_image.h,
tests/unit/test_tlv_auth.c and the rest of #93's TLV work.

Verified:
  end to end, with the real tool:
      eos_sign.py keygen / sign / verify  -> VERIFIED
      total 744 = 156 header + 512 payload + 76 TLV
      hdr_size 156, tlv_len 76, TLV magic 0x6907 at 156+512
      sha256(tlv)[:28] == header bytes 64..92          True
  ctest                                    22/22 PASS
  test_eos_sign_boot_path (real parser over real tool output):
      current layout [hdr][payload][tlv]: verify_integrity -> 0   PASS
      old layout     [hdr][tlv][payload]: verify_integrity -> -3  PASS
      TLV magic at addr + hdr_size + image_size: 0x6907           PASS
    that third line is finding 2's regression test -- it fails on the old
    layout, where the address lands past the end of the image.
  pytest tests/ (EOS_REQUIRE_SIGNING_TESTS=1)          44 passed
  the CI gate itself: with the variable set and the module absent, the import
    raises ImportError -> collection error -> job fails, rather than skipping.

Refs #88, #93
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants